Let's adapt our pull request management

I thought about this some more, especially PRs that are stale (no activity in a long time) and how we could use automation to help reviewers here.

If you sort the PRs by least recently updated https://github.com/openmrs/openmrs-core/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc

You can see at least a few that are a few years old (5 currently, I recently closed one). Dating back to 2018. They are not that many right now but it is a recurring thing. Developers sometimes move on to other projects, do not have the time to finish/address issues in a PR. So PRs sit there. Reviewers have to repeatedly ask whether the developer will continue, wait for an answer and at some point close it. Reviewers already have a lot on their hands. There are a lot of active PRs out there.

What if we could at least take away that work by automating it? I stumbled on GitHubs own stale bot

That we could integrate. We can customize at what point a PR is considered stale (for example half a year). We can also add a respectful, encouraging but honest message. Rough example:

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare! We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs. Your PR has not seen any activity since > half a year. This is why we have decided to close this PR for now. This allows us OpenMRS reviewers to focus our limited time to review all other PRs in a timely and professional manner.

Please feel free to reassign yourself to the issue you worked on in our JIRA when you have time to focus on it. After that reopen a new PR and we will be glad to work with you to get your contribution merged. Thank you very much for your help and understanding :slight_smile:

What are your thoughts?

If people think this is a good idea I will happily take on the task of implementing this. I will of course make sure the stale bot comment is tweaked. Volunteers could help me review the message itself.

3 Likes

@teleivo i like this initiative. :slight_smile:

Will the message change for pull requests where the lack of activity is caused by the pull request author waiting for reviewers?

Very good point :slight_smile:

The stale bot does not make a difference as to who’s turn it would be to respond/advance the PR.

However, it has the possibility to send a reminder with a grace period before closing the PR.

Example: after 5 months (configurable) of inactivity it comments on the PR with a message (configurable) like “This PR has not seen any activity in the last 5 months. We just wanted to check whether the you are still actively working on it or need any help so the change can be merged? Please respond by commenting on the issue if you would like to continue working on it or require assistance. If not we will close the PR in 4 weeks.”

This comment will notify everyone who is either subscribed to the repository, assigned to the PR, a reviewer to the PR or the actual commiter.

If then after 4 more weeks, so after 6 months in total (configurable) there has been no activity (no push, comment) on the PR it will be closed with a message (configurable; for example the one I posted above).

If there would be a comment/new commits it would be seen as active again and the counter would start at 0.

I believe the reminder with some time before actually closing it should be sufficient so that a PR in case it has not been reviewed in time by us gets more attention and close stale PRs where the developer has moved on :slight_smile:

And furthermore a close PR can always be brought back. The code is still on GitHub. So its easy for the commiter or us to bring it back :slight_smile:

1 Like

:+1: :+1: Automation plus bringing things to attention in a stepped-approach… what’s not to like here? (And keeping our PR backlog a bit shorter in theory.)

This is definitely a good start. And i would not object to implementing it as version 1.0.0 :slight_smile:

Though, for a volunteer, who has not yet received a single review, i some how feel that if we can make the automation more intelligent and start by doing some review or at least apologise for taking too long to review, this would be more encouraging than asking if they need any help so the change can be merged. For in this case, it is us who need help, not them. :smile:

1 Like

I totally agree!

I did some research/experimentation :slight_smile: It seems like the bot will be replaced with a GitHub action. As GitHub actions are here to stay, give us more control in general and are super easy to setup

Here is the action for it https://github.com/actions/stale

I am watching it for future improvements, its under active and open development. There are also other people thinking about your point https://github.com/actions/stale/issues/38

I will implement it. We can see how it feels and am open for any feedback.

Are you ok with the mark the PR as stale after 5 months and close it after 6 months as a first step? That at least to me sounds like a very relaxed timeframe.

I would at least give a second reminder before closing. :slight_smile:

Unfortunately the stale action or bot do not provide an automated second reminder.

The PRs will get a label Stale together with the configured comment (for example at 5 months). We can the label use to filter the PRs. Anyone can remove the filter which will mark the PR as not Stale again.

I would say the proposed timeframe is pretty relaxed . We can try it and see how it fits into our workflow. The author and potential reviewers all can take action. If that is not good enough we can also create a custom action that sends out another reminder if needed.

That would be more forgiving. :slight_smile:

but its also more work :joy:

What if we try with whats available and see how works out. I mean some devs simply move on. And adding it would at least give an automated notification. Currently there is no notification and some PRs just become stale. I believe it would be an improvement to the status quo :robot:

I adapted the comment to stale PRs to

OpenMRS welcomes your contribution! It means a lot to us that you want to contribute to equity in healthcare!

This PR has not seen any activity in the last 5 months. That is why we wanted to check whether you are still working on it or need assistance from our side.

Please note that this is an automated message and we might very well be the reason why there has not been any activity lately. We certainly do not want to discourage you from contributing. We do need to be honest in that OpenMRS has limited resources for reviewing PRs.

If you do not have time to continue the work or have moved on you don’t need to do anything. We will automatically close the PR in 30 days. We hope to see you back soon :slight_smile:

If you would like to continue working on it or require help from us please respond by commenting on the issue.

hoping this would resolve the point you had @dkayiwa that it could in fact be us not having responded/reviewed it :slight_smile:

Like i have stated before, this is our first attempt at this kind of automation where i would not expect perfection. So you have my vote to go ahead! :smile:

1 Like

How about a tl;dr at the top like “tl;dr our bot detected no activity on this PR and ”

Should they comment on an issue? Or the PR?

1 Like

Sure, I will work on an update of the comment :ok_hand:

Commenting or pushing any changes to the PR counts as an update. It works on GitHub PRs and GitHub issues. Since we are not using GitHub issues we will not use this feature.

The stale action is live :slight_smile: It labeled 6 PRs as stale (so not updated in the last 5 months) https://github.com/openmrs/openmrs-core/pulls?q=is%3Aopen+is%3Apr+label%3AStale

You can see the stale action logs at the actions tab. It runs as a crown job once every day. Debug output is enabled so we see why something was labeled. Example run https://github.com/openmrs/openmrs-core/runs/900729160?check_suite_focus=true

You might have already gotten an email or notification if you are subscribed to the PR. Here is an example of the current comment https://github.com/openmrs/openmrs-core/pull/2995#issuecomment-662756572 (its markdown so we can also adapt its style)

If a PR should not be closed after being labeled as stale it needs attention. That means remove the label and also review it, comment or push a change. The PR is shown like a timeline with events at specific points in time, so you can see when it was last updated. I think the bot comment does not count as an update.

A best practice for reviewers would be to sort PRs by least recently updated. So that not only newly created PRs get attention and older ones don’t get stale.

Let me know what you think. Feedback is more than welcome :slight_smile:

1 Like

I have got email notifications for the 6 pull requests. Thanks @teleivo for this initiative. :smile:

@teleivo Thanks for the thread, although the best way to reduce the number of PRs is to increase the number of potential reviewers that we the cycle time can be improved

hint hint @grace @jennifer @burke

3 Likes

@teleivo Thanks for this initiative! I’m wondering two things:

  1. Should we update our Wiki (somewhere) to describe the PR management process described in this thread?

  2. Should we maybe consider adopting something like this stale action for other OpenMRS repos?

2 Likes

This reminds me of the issue @bistenes identified with module maintenance: “Since everyone maintains everything, no one maintains anything.” We encourage everyone in the community to help manage PRs - and then we look to the same people who always do it because they have always done it. How can we go beyond encouraging “everyone” to manage PRs and instead motivate and empower specific individuals to take ownership of PR management?

I can think of a couple of ways to do this, building on some things that are already in motion:

  • One of the advantages of @bistenesmodule maintenance system is that it would actually lead to more “responsive on PRs. No old PRs collecting dust” because the module maintainers are responsible for reviewing, responding to, and closing PRs. Some module maintainers have already been identified - and are working on identifying others.

  • @burke, haven’t we also integrated PR management into our developer stages?

1 Like

Thanks @jennifer, yeah, this is a big part of what I had in mind with the module maintenance initiative.

Though to @ssmusoke’s point, which I think is important—yes, we need not only more organization, but also more potential reviewers. I’m hopeful that more mentorship, and something like a fellowship program, could help expand the pool of potential reviewers, and maybe also increase the quality of reviews generally.

Not sure if that’s what you meant @ssmusoke? But anyway that’s what stands out to me as a big opportunity.

1 Like