Improving Pull Requests workflow and merge time


#1

On average it takes about two weeks for a PR to get merged on theforeman/foreman, but that doesn’t mean that every PR is merged within that time, for a few it take two weeks or even longer till they have a first review/comment.

While there are many reasons why a PR takes time to merge, it should be important to give and get feedback as early as possible. For this reason I wanted to start a discussion on this topic, to see what can be done to improve.

There is already a GitHub template in theforeman/foreman when opening a pull request, which provides hints for requirements of a PR to submit. This could be extended with a few headlines and suggestions of information to include to make it easier for the PR to be tested and reviewed, like including rough steps to test and reproduce the issues/fix/feature. Or add hints what to add, that could increase the likelyhood of a PR being reviewed.

I think the best way to answer the question what can be done to increase the likelyhood of a PR being reviewed is to ask “What keeps you from reviewing?”


#2

I don’t mind having PRs untouched for few weeks, that happens and eventually when you poke people they will start a review. But what’s driving me crazy is when someone starts a review, I need to rebase the PR several times and then the PR gets lost and reviewer never returns back. I mean, a PR can be in non-mergeable state that’s fine but it must be obvious from reviewer comments because then both sides can wait forever.

I usually skip PRs where I see someone is making comments because this makes me think that someone is looking into the PR. This is lack of github feature - ability to see if a PR has an assigned reviewer in notifications without hovering (it shows it after mouse hover tho but with amount of PRs I don’t do that and make a wild guess).

We have a label called “Not yet reviewed” and I think this label is IMHO useless, the bot removes it the moment a PR is merged, so it’s a label that is present while a PR is opened. That’s zero information and actually misleading one - reviewers rarely remove this flag. At least I forget to do this all the time. We should be looking at “Review required” built-in flag only.