Unresolved Review Threads
On February 4th 2021, GitHub publicly released an auto-merge feature. This feature was already available in beta version since December 16th 2020. Using this feature, as you would have guessed, you can tell GitHub to automatically merge your pull request once all the requirements are met.
The concept of auto-merge is far from new and some solutions were already available on the market. To me, this kind of feature is really relevant in a professional context in which there is a rigorous review process between the development team members.
However, from all the solutions I have been exposed to, I have observed the same weakness: THERE IS NO WAY TO PREVENT THE MERGE OF A PULL REQUEST HAVING UNRESOLVED REVIEW THREADS.
In response to this release, I decided to get my hands dirty by creating my very first GitHub Action which aim at preventing the merge of a pull request with unresolved review threads.
In the following article, I will expose different thoughts I had along the way.
Historically
Historically, I had the habit of integrating a third-party action (pascalgn/automerge-action) or a bot (palantir/bulldozer) to achieve the same goal. From now on, a simple check box can be checked instead 🎉. You can look at GitHub documentation for more details. For the supporters of the probot/settings initiative, there is also an opened issue at the moment in order to make this setting configurable “as code”.
Philosophy
Having a rigorous review process is primordial in order to improve code quality, increase knowledge sharing, and promote code collective ownership. However, it also comes to the price of introducing some latency into the development flow since all the members do not necessarily have the same work schedule/habits.
Since the very first day I have been introduced to the concept of auto-merge, I always have been on the lookout in order to prevent the merge of a pull request with unresolved review threads. Too often, I have seen the same scenario happening.
Alice: Open a pull request and mark it as automatically mergeable
Bob: Review Alice’s pull request, leave some non-blocking comments that should still be addressed before merge, and accept in advance the pull request trusting Alice to perform the desired corrections (supposing that when new commits are pushed approvals won’t be dismissed)
GitHub: Merge the pull request…
Bob: Realize that he forgot to check if the pull request was marked as automatically mergeable
Alice: Come back after lunch having to recreate a new branch with only the desired corrections, but this time without the surrounding context and purpose
I am pretty sure that some of you recognized themselves while reading the above scenario. Even if the consequences are minor in most situations, it is still irritating to experience it. Every time it happened, I complained out loud,
Why is there no way to prevent this situation? It should not be that hard to implement!
Even if I achieved to create an action to solve this problem, I consider my solution far from perfect and now I know why I was not able to find such a check.
Obtained Solution
So let’s dig deeper into the obtained solution and the expected flow of usage. The proposed solution rely on the usage of labels to manually trigger the check once all review threads resolved. I also added the possibility to bypass the check in the case there would be a pull request with lots of unresolved review threads that you do not want to resolve, but still want to merge your pull request.
How it is intended to be used?
Make sure the
Unresolved Review Threadscheck is required into your branch protection rules.
- Open a pull request
Will trigger flow naturally and mark pull request as passing
- Request for review
- Enable auto-merge on pull request
- Reviewers leave comments
Will trigger flow and mark pull request as failing
- Reviewers may approve pull request at that point if the raised concerns are not blocking/major
- Discuss raised comments
- Update pull request
- Mark review threads as resolved
- When you are done resolving comments, remove the unresolved label to trigger flow manually
- if no unresolved review thread is found, the pull request will be marked as passing
- If auto-merge was enabled and all checks are passing, the pull request is merged
Get Started
If you want to try the action in your project, simply create a .github/workflows/unresolved-review-threads.yml file and copy/paste the following content in it.
Limitations
As of right now, the obtained flow is the combination of multiple limitations. The most important one is the fact that there is no way to trigger a GitHub Action when a review thread comment is resolved/unresolved. At this moment, I knew I would be constrained to include some sort of manual trigger to re-validate the presence of unresolved threads on-demand. There is an opened ticket in the GitHub Community that you can go upvote to help the cause.
At first, I thought it would be nice to create lots of different triggers using labels, comments, reactions, etc. I rapidly faced some issues doing so and this made the code a lot more complicated that expected. Understandingly, events’ payload vary depending on the type of event triggered. This is another lesson I have learned along the way. Even if there is a way to make it works, I thought that the investment was not worth at the time being and it would be preferable to wait for an external feature request to support it. So I came back to a simpler version working exclusively with labels which was IMHO more user friendly.
Nevertheless, as you can see above, all these workarounds forced me to make the action respond on many different triggers to make this action as seamless to use as possible. Which brings me to another observation I made about the different GitHub triggers: there are not mutually exclusive and can lead to multiple logical triggers of your action for a single user interaction performed. Such an example is the overlap between pull_request_review [submitted] and the pull_request_review_comment [created].
Finally, I spend some time figuring out how to properly report the check to the pull request. Usually, when creating a traditional pull_request only action, you can simply use core.setFailed() method to report a failure. However, the moment you start executing your action using non pull_request trigger, this approach won’t work anymore. I then tried really hard to update the pull_request check suite status of my action “on place”. This approach might have been a possible workaround, but it started to get messy the moment I had multiple check suite runs of my action with no way to identify the right one to update. At the end of the day, the only viable way I found was to create a stand alone commit status check next to the action flow run. But, even with this approach, I never success to mark the commit check as pending while the check was executing…
Conclusion
Overall, I am happy of the result since I can now be assured that no pull request will be merged with unresolved review threads and all concerns will be addressed while still making the review process as asynchronous as possible. I do hope my work will help some of you as well and that GitHub will improve the exposed triggers or implement their own status check for unresolved review threads. Nevertheless, I learned a lot during the process and I consider it was a fun little project to work on during my spare time. Feel free to ask for new features or document bugs if you find some.
