Let's adapt our pull request management

Tags: #<Tag:0x00007f01bc2d5bd0> #<Tag:0x00007f01bc2d5ae0> #<Tag:0x00007f01bc2d59f0>

Hi there :github:

for quite some time now I observe that a large number of PRs are piling up on openmrs-core (steady > 30 for some months now). We even have some from end of 2017 which is not a good indicator of our projects health. I hope we do not loose contributors due to our slow response time.

I have a few suggestions that might help

  • Please assign yourself to PRs: this way other reviewers know that you are taking care of this specific PR. You can setup a bookmark in your browser to a filtered PR list with only PRs you are assigned to and give feedback on a regular basis. Github now also shows these in your news feed when you are logged in and go to github.com see tab “browse activity”. See on how to assign yourself https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/ This is for example how my activity looks like

  • Use Githubs saved replies: take the time to setup well crafted answers to issues you often see on PRs. Add links to our wikis/other resources that can be of help. Say why you would like to have something changed. This creates understanding on the contributor part/or starts discussions and also spreads knowledge. This will save you a lot of time, help you not get frustrated or burned out and provide better reviews for our contributors. See maintainer happiness: github saved replies

  • Close PRs if you feel that someone does not respond. I just closed a couple of PRs that have not seen activity in a few months, after I asked again and again if they are still working on it. But mention that its just to keep our reviewers focus and they should reopen a new PR if they want to continue their work.

  • Try to setup a little bit of time to regularly review the PRs you assigned yourself to

  • Do not assign yourself to 10 PRs, start with 1 and see it through to the end

  • Ask for help if you are unsure. You can ask for another reviewer on the right of a PR

41

  • As a reviewer and someone creating PRs, be mindful of other peoples time. A lot of us are volunteers, or managing several projects. Lets keep this community a safe place we like to work in :blush:

Thank you all for your great work, time and effort you have and are putting in :sunny:

3 Likes

@teleivo this is awesome!!! :smile:

Hi there :slight_smile:

the topic came to my attention again and then I found an old post of mine so I thought why not bump.

We currently have 130 open PRs and when I filter for Assignee then click Assigned to nobody https://github.com/openmrs/openmrs-core/pulls?q=is%3Aopen+is%3Apr+no%3Aassignee we have 129.

This feature of assigning yourself to a PR https://help.github.com/en/github/managing-your-work-on-github/filtering-issues-and-pull-requests-by-assignees

got even more useful with the improvements to github. You can go to https://github.com/pulls and easily follow up on the PRs you are reviewing.

Please try it out. This would make it clear to everyone that a PR already has people looking into it and you could pick another one. I believe this can help us focus our efforts/time.

I also believe we should not be afraid of closing old PRs see my first post.

Let me know what you think of these features/proposal and if it helped you. Also please share if you tried it and do not believe it adds any benefit.

Thanks @teleivo for bringing this back and the numerous code reviews that you are doing, of late. Welcome back! :smile:

Since we encourage reviews from as many people as possible, would there be a need to look elsewhere because some one is already reviewing? :slight_smile:

1 Like

Good point :slight_smile: Multiple reviewers is a good thing! This feature can at least be used to see what PRs have no one assigned and need attention. It can also help reviewers manage the PRs they are working on. It should definitely not discourage anyone from reviewing a PR that has already been reviewed.

1 Like

I completely agree with you on this! :slight_smile:

Thanks @teleivo that’s awesome, I didn’t know that one!

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