Our team has been using GitHub to review the code for our open-source product. We have encountered several issues with GitHub code reviews. The default GitHub code review process is not scalable and provides a poor developer experience.
How to set up a GitHub code review process
GitHub admins can create a branch protection rule that requires a code review before merging the code to the main branch.
Here’s a representative branch protection rule:
PR branch protection rule
When a developer creates a pull request, GitHub requires code reviews from all relevant owners specified by the
CODEOWNERS file in the repository. If someone makes a new push to the PR, all the owners need to re-approve the PR.
In a previous article, we covered how to find the required code owners for a PR. This is another issue, but we will not discuss it in this article.
Issue 1: CODEOWNERS is not scalable
GitHub uses a
CODEOWNERS file to define individuals or teams responsible for each file in the repository.
The CODEOWNERS file format favors fine-grained code ownership, where the last matching pattern takes precedence over
previous patterns. Here is an example from the GitHub documentation:
# In this example, @octocat owns any file in the `/apps`
# directory in the root of your repository except for the `/apps/github`
# subdirectory, as this subdirectory has its own owner @doctocat
/apps/ @octocat
/apps/github @doctocat
The CODEOWNERS file is not scalable for medium-to-large and even small organizations. As the number of code owners
grows, each pull request is likely to require approval from more code owners. Each code owner may request changes,
potentially leading to cycles and cycles of re-approvals.
Tracking down multiple people to approve and re-approve a PR can be time-consuming and frustrating for developers. This results in longer PR turnaround times, slower development velocity, and missed commitments.
From a developer experience perspective, we want to make the code review process as smooth and efficient as possible,
which means one reviewer for one PR. This approach is feasible by manually inverting the
last matching pattern takes precedence rule in the CODEOWNERS file by always including the owner(s) from the
previous pattern. For example, we would rewrite the above owners as:
/apps/ @octocat
/apps/github @octocat @doctocat
Keeping the CODEOWNERS file in this format may be cumbersome to do manually, but it can be done with a script.
Issue 2: Re-approvals for every push
When a developer makes a new push to a PR, all the code owners need to re-approve it. This is a poor developer experience, as it requires the code owners to review potentially the same code changes multiple times.
The issue stems from the lack of fine-grained control over the following option:
With multiple code owners, every code owner must re-approve every change.
A code owner should not need to re-review code that didn’t change – this is a waste of time and effort.
With a single code owner, the reviewer must re-approve trivial or irrelevant changes, such as:
- fixing a typo in a comment
- fully accepting a suggestion from the reviewer
- re-generating an auto-generated file, such as documentation
The required re-approvals can be frustrating and time-consuming for developers and code owners. They make developers feel untrusted and inefficient.
The main argument for requiring re-approvals is security—we don’t want to merge potentially malicious code. If that’s the case, we should have a security review process in place, not a code review process. A security review can be done by a separate individual and improved by automated tools.
In addition, we should be able to completely exclude some files/directories from the code review process. For example, generated files, such as documentation based on code changes, should not require code review. Other generated files, such as testing mocks, may have CI/CD checks that ensure they are generated correctly, and they should not require code review either.
Issue 3: Impractical to maintain a protected feature branch
A protected feature branch requires code reviews before merging. Since all the commits on the feature branch have already been reviewed and approved, it is considered safe to merge into the main branch.
The main issue is that the developer cannot simply update this feature branch with the latest changes on the main branch. They need PR approval from all the code owners who have already approved the same changes on the main branch. This busy work is another example of a waste of time and effort.
In addition, a feature branch may be long-lived and introduce changes across multiple areas of the code base. This means that it may require approval from many code owners, which can be time-consuming and frustrating.
Solution: Custom GitHub Action to manage code reviews
Instead of relying on the default GitHub code review process, we can create a custom GitHub Action to manage code reviews. The custom GitHub Action can:
- automatically identify a single reviewer for a PR (or identify a small group of reviewers, each of whom can approve the PR)
- automatically exclude specific files/directories from the code review process
- automatically maintain the approval state of the PR when new commits meeting explicit criteria are pushed
- enable a usable and practical protected feature branch
In a future article, we will explore how to create a custom GitHub Action to manage code reviews.
Further reading
Watch the top 3 issues with GitHub code reviews
Note: If you want to comment on this article, please do so on the YouTube video.