Several teams at my company practice a code review workflow I’ve never seen before. I am trying to understand the thinking behind it, with the idea that there’s value in making the whole company consistent. (I contribute to multiple codebases and have been tripped up by the differences in the past.)
- Code author submits a pull request
- Reviewer examines the code
-
- If the reviewer approves, they leave a comment along the lines of “Looks good, feel free to merge”
- If the reviewer has concerns, they leave a comment like “Please fix minor issues X and Y, then merge” (For major changes, return to step 2)
- The code author makes changes if necessary, and then merges his or her own pull request
I have the following concerns:
-
In the case of approval at step 3, this workflow creates a seemingly-unnecessary roundtrip to the pull request author. The reviewer, who is already looking at the code, could just merge it immediately.
-
In the case of changes being requested at step 3, the agency to merge the pull request now rests solely with the PR’s author. No one besides the author will look at the changes prior to merging.
What are some other advantages or disadvantages to this workflow? Is this workflow common on other engineering teams?
4
In the first case, it’s usually a courtesy. In most organizations, merges kick off a series of automated tests which must be dealt with promptly if they fail. Especially if there was a significant delay between when a pull request was submitted and when it was reviewed, it’s polite to allow it to be merged on the author’s timetable, so they have time to deal with any unexpected fallout. The easiest way to do that is to let them merge it themselves.
Also, sometimes the author becomes aware of reasons later that a pull request shouldn’t be merged yet. Maybe another developer’s PR is higher priority and would cause conflicts. Maybe she thought of an uncovered use case. Maybe a review comment triggered a brainstorm that needs further investigation before the issue is fully satisfied. The author knows the most about the code, and it makes sense to give him or her the last word about when it gets merged.
On the second point, that’s just a matter of trust. If you can’t trust people to fix minor issues without being double-checked, they shouldn’t be working for you. If the issue is big enough that it will need another review after the fix, then trust reviewers to ask for one.
That being said, I do occasionally merge other author’s pull requests, but it’s usually either very simple changes, or from external sources, where I personally take responsibility for shepherding through any test automation failures.
3
Having the initial author merge their own pull request is my preferred workflow in small teams. In addition to technical advantages already mentioned (in terms of resolving merge conflicts, for example), I think it adds value on a cultural level: It builds a sense of ownership.
I specified initial author for the (rare) case when another developer would add commits to the open pull request (pulling the work-in-progress feature branch and pushing back to it). This does not happen often, and would result from a conversation in person or over Slack: These additional commits (by someone else) should not land there by surprise! In this context, by initial author, I mean the one who submitted the pull request.
2
In my organization we are quite new at pull requests and your question is one I have pondered myself.
One observation I would like to add: In some tools (we use TFS) there might be a work item associated with the pull request.
If that is the case, it becomes a bit of a hassle to keep track of when the reviewer performed the merge. In that scenario the developer then has to revisit the PR, open the bug or change request and mark it as ‘resolved’. If we mark it ‘resolved’ too early, the testers believe that the fix is already part of the current build.
TFS 2017 improved on their implementation of the pull request. Now the developer can request the Pull Request to automatically merge if all the conditions are met (no merge conflict, approval from reviewers and no broken builds). YMMV.
This way, the author has a chance to change his mind about his branch, maybe he’s figured out something that should be done in a different way, and put the additional charges back up for review.
Added much later: You should only do a merge that can be done without merge conflicts. To achieve this, I merge the baseline into my code first – the result of that would later become the new baseline by merging it to the baseline without merge conflicts.
If there are merge conflicts then I fix them and the merged result goes into the pull request. So the pull request contains only the differences between my branch and the latest base line, and when the merge is eventual done, the merged result is exactly what has been on my machine.
The author, in most cases, knows more about the code than the reviewer. They are also the ones responsible for that piece of code. If something goes wrong, the author, who is familiar with that part of the code, can jump in and fix it. It creates a sense of responsibility and ownership for the authors.
Also, at my job, we have CI/CD set up for testing and deploys, but we don’t really have it set up for DB migrations. That falls to the developer to run migrations when it gets deployed.
It’s important to allow them to merge at their preferred time in order to run migrations or fix bugs/tests.
Edit: (thanks FluidCode)
The reviewer is responsible for checking the quality of the code i.e. is it readable, follows a standard approach, catch obvious inefficiencies, ensure tests were added, and in some high profile companies (like banks) ensure there aren’t any attempted fraud.
1