doc/development/code_review.md
This guide contains advice and best practices for performing code review, and having your code reviewed.
All merge requests for GitLab CE and EE, whether written by a GitLab team member or a wider community member, must go through a code review process to ensure the code is effective, understandable, maintainable, and secure.
Before you begin:
As soon as you have code to review, have the code reviewed by a reviewer. This reviewer can be from your group or team, or a domain expert. The reviewer can:
If the merge request is small and straightforward to review, you can skip the reviewer step and directly ask a maintainer.
What constitutes "small and straightforward" is a gray area. Here are some examples of small and straightforward changes:
Otherwise, a merge request should be first reviewed by a reviewer in each category (for example: backend, database) the MR touches, as maintainers may not have the relevant domain knowledge. This also helps to spread the workload.
For assistance with security scans or comments, include the Application Security Team (@gitlab-com/gl-security/appsec).
The reviewers use the reviewer functionality in the sidebar. Reviewers can add their approval by approving additionally.
Depending on the areas your merge request touches, it must be approved by one or more maintainers. The Approved button is in the merge request widget.
Getting your merge request merged also requires a maintainer. If it requires more than one approval, the last maintainer to review and approve merges it.
Some domain areas (like Verify) require an approval from a domain expert, based on
CODEOWNERS rules. Because CODEOWNERS sections are independent approval rules, we could have certain
rules (for example Verify) that may be a subset of other more generic approval rules (for example backend).
For a more efficient process, authors should look for domain-specific approvals before generic approvals.
Domain-specific approvers may also be maintainers, and if so they should review
the domain specifics and broader change at the same time and approve once for
both roles.
Read more about author responsibilities below.
Domain experts are team members who have substantial experience with a specific technology, product feature, or area of the codebase. Team members are encouraged to self-identify as domain experts and add it to their team profiles.
When self-identifying as a domain expert, it is recommended to assign the MR changing the .yml file to be merged by an already established Domain Expert or a corresponding Engineering Manager.
We make the following assumption with regards to automatically being considered a domain expert:
We default to assigning reviews to team members with domain expertise for code reviews. UX reviews default to the recommended reviewer from the Review Roulette. Due to designer capacity limits, areas not supported by a Product Designer will no longer require a UX review unless it is a community contribution. When a suitable domain expert isn't available, you can choose any team member to review the MR, or follow the Reviewer roulette recommendation (see above for UX reviews). Double check if the person is OOO before assigning them.
To find a domain expert:
git log <file>.git log <file>.https://gitlab.com/gitlab-org/gitlab/-/commit/<SHA>.[!note] Reviewer roulette is an internal tool for use on GitLab.com, and not available for use on customer installations.
The Danger bot randomly picks a reviewer and a maintainer for each area of the codebase that your merge request seems to touch. It makes recommendations for developer reviewers and you should override it if you think someone else is a better fit.
Approval Guidelines can help to pick domain experts.
We only do UX reviews for MRs from teams that include a Product Designer. User-facing changes from these teams are required to have a UX review, even if it's behind a feature flag. Default to the recommended UX reviewer suggested.
It picks reviewers and maintainers from the list at the engineering projects page, with these behaviors:
It doesn't pick people whose Slack or GitLab status:
OOO, PTO, Parental Leave, Friends and Family, or Conference.palm_tree, 🏖️ beach, ⛱ beach_umbrella, 🏖 beach_with_umbrella, 🌞 sun_with_face, 🎡 ferris_wheel, 🏙 cityscapethermometer, 🤒 face_with_thermometerIt doesn't pick people who are already assigned a number of reviews that is equal to or greater than their chosen "review limit". The review limit is the maximum number of reviews people are ready to handle at a time. Set a review limit by using one of the following as a Slack or GitLab status:
twothreefourfiveThe minimum review limit is 2️⃣. The reason for not being able to completely turn oneself off for reviews has been discussed in this issue.
Review requests for merge requests that do not target the default branch of any project under the security group are not counted. These MRs are usually backports, and maintainers or reviewers usually do not need much time reviewing them.
It always picks the same reviewers and maintainers for the same
branch name (unless their out-of-office (OOO) status changes, as in point 1). It
removes leading ce- and ee-, and trailing -ce and -ee, so
that it can be stable for backport branches.
People whose Slack or GitLab status emoji
is Ⓜ :m:are only suggested as reviewers on projects they are a maintainer of.
The Roulette dashboard contains:
As described in the section on the responsibility of the maintainer below, you are recommended to get your merge request approved and merged by maintainers with domain expertise. The optional approval of the first reviewer is not covered here. However, your merge request should be reviewed by a reviewer before passing it to a maintainer as described in the overview section.
| If your merge request includes | It must be approved by a |
|---|---|
~backend changes <sup>1</sup> | Backend maintainer. |
~database migrations or changes to expensive queries <sup>2</sup> | Database maintainer. Refer to the database review guidelines for more details. |
~workhorse changes | Workhorse maintainer. |
~frontend changes <sup>1</sup> | Frontend maintainer. |
~UX user-facing changes <sup>3</sup> | Product Designer. Refer to the design and user interface guidelines for details. |
| Adding a new JavaScript library <sup>1</sup> | - Frontend Design System member if the library significantly increases the bundle size. |
More information about license compatibility can be found in our GitLab Licensing and Compatibility documentation. | | A new dependency or a file system change | - Distribution team member. See how to work with the Distribution team for more details.
~documentation or ~UI text changes | Technical writer based on assignments in the appropriate DevOps stage group. |
| Changes to development guidelines | Follow the review process and get the approvals accordingly. |
| End-to-end and non-end-to-end changes <sup>4</sup> | Software Engineer in Test. |
| Only End-to-end changes <sup>4</sup> or if the MR author is a Software Engineer in Test | Quality maintainer. |
| A new or updated application limit | Product manager. |
| Analytics Instrumentation (telemetry or analytics) changes | Analytics Instrumentation engineer. |
| An addition of, or changes to a Feature spec | Quality maintainer or Quality reviewer. |
| A new service to GitLab (Puma, Sidekiq, Gitaly are examples) | Product manager. See the process for adding a service component to GitLab for details. |
| Changes related to authentication | Manage:Authentication. Check the code review section on the group page for more details. Patterns for files known to require review from the team are listed in the in the Authentication section of the CODEOWNERS file, and the team will be listed in the approvers section of all merge requests that modify these files. |
| Changes related to custom roles or policies | Manage:Authorization Engineer. |Specs other than JavaScript specs are considered ~backend code. Haml markup is considered ~frontend code.
However, Ruby code in Haml templates is considered ~backend code. When in doubt, request both a frontend and
backend review.
For Haml template changes specifically:
project_id: @project&.to_global_id) would benefit from a backend review for Ruby logic
correctness and frontend review for the component integration.We encourage you to seek guidance from a database maintainer if your merge request is potentially introducing expensive queries. It is most efficient to comment on the line of code in question with the SQL queries so they can give their advice.
User-facing changes include both visual changes (regardless of how minor), and changes to the rendered DOM which impact how a screen reader may announce the content. Groups that do not have dedicated Product Designers do not require a Product Designer to approve feature changes, unless the changes are community contributions.
End-to-end changes include all files in the qa directory.
Some merge requests require mandatory approval by specific groups.
See .gitlab/CODEOWNERS for definitions.
Mandatory sections in .gitlab/CODEOWNERS should only be limited to cases where
it is necessary due to:
When adding a mandatory section, you should track the impact on the new mandatory section on merge request rates. See the Verify issue for a good example.
All other cases should not use mandatory sections as we favor responsibility over rigidity.
Additionally, the current structure of the monolith means that merge requests are likely to touch seemingly unrelated parts. Multiple mandatory approvals means that such merge requests require the author to seek approvals, which is not efficient.
Efforts to improve this are in:
This checklist encourages the authors, reviewers, and maintainers of merge requests (MRs) to confirm changes were analyzed for high-impact risks to quality, performance, reliability, security, observability, and maintainability.
Using checklists improves quality in software engineering. This checklist is a straightforward tool to support and bolster the skills of contributors to the GitLab codebase.
For further quality guidelines, see testing.
~ux, ~frontend, ~backend, and ~database labels accordingly.Flaky test '<path/to/test>' was found in the list of files changed by this MR. but can be in jobs that pass with warnings.~security label and you have @-mentioned @gitlab-com/gl-security/appsec.@gitlab-com/gl-security/appsec.The responsibility to find the best solution and implement it lies with the merge request author. The author or directly responsible individual (DRI) stays assigned to the merge request as the assignee throughout the code review lifecycle. If you are unable to set yourself as an assignee, ask a reviewer to do this for you.
Before requesting a review from a maintainer to approve and merge, they should be confident that:
The best way to do this, and to avoid unnecessary back-and-forth with reviewers, is to perform a self-review of your own merge request, following the Code Review guidelines. During this self-review, try to include comments in the MR on lines where decisions or trade-offs were made, or where a contextual explanation might aid the reviewer in more easily understanding the code.
To reach the required level of confidence in their solution, an author is expected to involve other people in the investigation and implementation processes as appropriate.
They are encouraged to reach out to domain experts to discuss different solutions or get an implementation reviewed, to product managers and UX designers to clear up confusion or verify that the end result matches what they had in mind, to database specialists to get input on the data model or specific queries, or to any other developer to get an in-depth review of the solution.
If you know you'll need many merge requests to deliver a feature (for example, you created a proof of concept and it is clear the feature will consist of 10+ merge requests), consider identifying reviewers and maintainers who possess the necessary understanding of the feature (you share the context with them). Then direct all merge requests to these reviewers. The best DRI for finding these reviewers is the EM or Staff Engineer. Having stable reviewer counterparts for multiple merge requests with the same context improves efficiency.
If your merge request touches more than one domain (for example, Dynamic Analysis and GraphQL), ask for reviews from an expert from each domain.
If an author is unsure if a merge request needs a domain expert's opinion, then that indicates it does. Without it, it's unlikely they have the required level of confidence in their solution.
Before the review, the author is requested to submit comments on the merge request diff alerting the reviewer to anything important as well as for anything that demands further explanation or attention. Examples of content that may warrant a comment could be:
If there are any projects, snippets, or other assets that are required for a reviewer to validate the solution, ensure they have access to those assets before requesting review.
When assigning reviewers, it can be helpful to:
~backend review and a ~database
review. While assigning the reviewers, the author adds a comment to the MR
letting each reviewer know which domain they should review.@ mentioning a maintainer in a comment is acceptable and in all other cases adding a reviewer is sufficient.This saves reviewers time and helps authors catch mistakes earlier.
Reviewers are responsible for reviewing the specifics of the chosen solution.
If you are unavailable to review an assigned merge request within the Review-response SLO:
This demonstrates a bias for action and ensures an efficient MR review progress.
Add a comment like the following:
Hi <@mr-author>, I'm unavailable for review but I've [spun the roulette wheel](https://gitlab-org.gitlab.io/gitlab-roulette/) for this project and it has selected <@new-reviewer>.
@new-reviewer may you please review this MR when you have time? If you're unavailable, please [spin the roulette wheel](https://gitlab-org.gitlab.io/gitlab-roulette/) again and select and assign a new reviewer, thank-you.
/assign_reviewer <@new-reviewer>
/unassign_reviewer me
Review the merge request thoroughly.
Verify that the merge request meets all contribution acceptance criteria.
Some merge requests may require domain experts to help with the specifics. Reviewers, if they are not a domain expert in the area, can do any of the following:
You should guide the author towards splitting the merge request into smaller merge requests if it is:
The author may choose to request that the current maintainers and reviewers review the split MRs or request a new group of maintainers and reviewers.
If the author has added local verification steps, indicate if you did these so the maintainer knows whether they were done, and what the result was.
When you are confident that it meets all requirements, you should:
@ mention the author to generate a to-do notification, and advise them that their merge request has been reviewed and approved.Maintainers are responsible for the overall health, quality, and consistency of the GitLab codebase, across domains and product areas.
Consequently, their reviews focus primarily on things like overall architecture, code organization, separation of concerns, tests, DRYness, consistency, and readability.
Because a maintainer's job only depends on their knowledge of the overall GitLab codebase, and not that of any specific domain, they can review, approve, and merge MRs from any team and in any product area.
Maintainers are the DRI of assuring that the acceptance criteria of a merge request are reasonably met. In general, quality is everyone's responsibility, but maintainers of an MR are held responsible for ensuring that an MR meets those general quality standards.
This includes avoiding the creation of technical debt in follow-up issues.
If a maintainer feels that an MR is substantial enough, or requires a domain expert, maintainers have the discretion to request a review from another reviewer, or maintainer. Here are some examples of maintainers proactively doing this during review:
Maintainers do their best to also review the specifics of the chosen solution before merging, but as they are not necessarily domain experts, they may be poorly placed to do so without an unreasonable investment of time. In those cases, they defer to the judgment of the author and earlier reviewers, in favor of focusing on their primary responsibilities.
If a developer who happens to also be a maintainer was involved in a merge request as a reviewer, it is recommended that they are not also picked as the maintainer to ultimately approve and merge it.
Maintainers should check before merging if the merge request is approved by the
required approvers.
If still awaiting further approvals from others, @ mention the author and explain why in a comment.
Certain merge requests may target a stable branch. For an overview of how to handle these requests, see the patch release runbook.
After merging, a maintainer should stay as the reviewer listed on the merge request.
Our code review process dogfoods the Merge request reviews feature. Here is a summary, which is also reflected in other sections.
:user_id?")dangerbot.200 lines is a good goal.
A -> B -> C), have their branches target each other instead of master. For example, have C target B, B target A, and A target master. This way each MR will have only their corresponding diff.frontend only, and later we can request backend review for the server request (see "splitting MRs" above).Keep in mind that code review is a process that can take multiple iterations, and reviewers may spot things later that they may not have seen the first time.
@
mention the reviewer instead.When GitLab Duo provides automated code review comments on your merge request, address all comments before requesting a review from human reviewers.
Your response can take any form, as long as it clearly indicates that you have considered the comment:
Leave GitLab Duo discussion threads unresolved so that reviewers and maintainers can easily see your responses and verify that all automated feedback has been appropriately addressed.
When you are ready to have your merge request reviewed, you should request an initial review by selecting a reviewer based on the approval guidelines.
When a merge request has multiple areas for review, it is recommended you specify which area a reviewer should be reviewing, and at which stage (first or second).
This will help team members who qualify as a reviewer for multiple areas to know which area they're being requested to review.
For example, when a merge request has both backend and frontend concerns, you can mention the reviewer in this manner:
@john_doe can you please review ~backend? or @jane_doe - could you please give this MR a ~frontend maintainer review?
You can also use workflow::ready for review label. That means that your merge request is ready to be reviewed and any reviewer can pick it. It is recommended to use that label only if there isn't time pressure and make sure the merge request is assigned to a reviewer.
When re-requesting a review, click the Re-request a review icon ({{< icon name="redo" >}}) next to the reviewer's name, or use the /request_review @user quick action.
This ensures the merge request appears in the reviewer's Reviews requested section of their merge request homepage.
When your merge request receives an approval from the first reviewer it can be passed to a maintainer. You should default to choosing a maintainer with domain expertise, and otherwise follow the Reviewer Roulette recommendation or use the label ready for merge.
Sometimes, a maintainer may not be available for review. They could be out of the office or at capacity. You can and should check the maintainer's availability in their profile. If the maintainer recommended by the roulette is not available, choose someone else from that list.
It is the responsibility of the author for the merge request to be reviewed. If it stays in the ready for review state too long it is recommended to request a review from a specific reviewer.
GitLab engineers who have capacity can regularly check the list of merge requests to review and add themselves as a reviewer for any merge request they want to review.
Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then:
Try to be thorough in your reviews to reduce the number of iterations.
Communicate which ideas you feel strongly about and those you don't.
Identify ways to simplify the code while still solving the problem.
Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
Seek to understand the author's perspective.
Check out the branch, and test the changes locally. You can decide how much manual testing you want to perform.
true in the code) rather than full environment setup.Your testing might result in opportunities to add automated tests.
If you don't understand a piece of code, say so. There's a good chance someone else would be confused by it as well.
Ensure the author is clear on what is required from them to address/resolve the suggestion.
Ensure there are no open dependencies. Check linked issues for blockers. Clarify with the authors if necessary. If blocked by one or more open MRs, set an MR dependency.
After a round of line notes, it can be helpful to post a summary note such as "Looks good to me", or "Just a couple things to address."
Let the author know if changes are required following your review.
[!warning] If the merge request is from a fork, also check the additional guidelines for community contributions.
Before taking the decision to merge:
At least one maintainer must approve an MR before it can be merged. MR authors and people who add commits to an MR are not authorized to approve the MR and must seek a maintainer who has not contributed to the MR to approve it. In general, the final required approver should merge the MR.
Scenarios in which the final approver might not merge an MR:
If any of these scenarios occurs, an MR author may merge their own MR if it has all required approvals and they have merge rights to the repository. This is also in line with the GitLab bias for action value.
This policy is in place to satisfy the CHG-04 control of the GitLab Change Management Controls.
To implement this policy in gitlab-org/gitlab, we have enabled the following
settings to ensure MRs get an approval from a top-level CODEOWNERS maintainer:
To update the code owners in the CODEOWNERS file for gitlab-org/gitlab, follow
the process explained in the code owners approvals handbook section.
Some actions, such as rebasing locally or applying suggestions, are considered
the same as adding a commit and could reset existing approvals. Approvals are not removed
when rebasing from the UI or with the /rebase quick action.
When ready to merge:
[!warning] If the merge request is from a fork, also check the additional guidelines for community contributions.
Thanks to merged results pipelines, authors no longer have to rebase their
branch as frequently anymore (only when there are conflicts) because the Merge
Results Pipeline already incorporate the latest changes from main.
This results in faster review/merge cycles because maintainers don't have to ask
for a final rebase: instead, they only have to start a MR pipeline and set auto-merge.
This step brings us very close to the actual Merge Trains feature by testing the
Merge Results against the latest main at the time of the pipeline creation.
[!warning] Review all changes thoroughly for malicious code before starting a merged results pipeline.
When reviewing merge requests added by wider community contributors:
Gemfile.lock or yarn.lock might appear trivial, they could lead to the
fetching of malicious packages.@gitlab-com/gl-security/appsec to review the merge request before manually starting any merge request pipeline.When reviewing merge requests from forked repositories, you have several methods to test the changes locally:
Use the GitLab CLI:
glab mr checkout <MR_ID>
For more information, see the GitLab CLI commands.
Use cURL to apply the diff. Use one of the following methods:
Fetch and apply the diff directly:
curl --silent <MR_URL>.diff | git apply
Replace <MR_URL> with the full URL to the merge request.
For example: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1234.diff.
Create a Git alias to avoid typing the full URL each time:
git config --global alias.mr '!f() { curl --silent "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/$1.diff" | git apply; }; f'
After the alias is created, use it with just the merge request ID. For example:
git mr 1234
Use GDK switch:
gdk switch <MR_ID>
[!note] This command also runs
gdk update, which updates your development environment. The process can take several minutes to complete.
For more information, see the
GDK switch.rb file.
If you frequently need to checkout upstream branches, you can add the canonical project/fork as a remote:
git remote add upstream https://gitlab.com/<canonical group>/<canonical project>.git
Then:
git fetch upstream
git checkout upstream/<remote-branch-name>
When an MR needs further changes but the author is not responding for a long period of time, or is unable to finish the MR, GitLab can take it over. A GitLab engineer (generally the merge request coach) will:
~"coach will finish" to their MR.~"Community contribution".One of the most difficult things during code review is finding the right balance in how deep the reviewer can interfere with the code created by a author.
GitLab is used in a lot of places. Many users use our Omnibus packages, but some use the Docker images, some are installed from source, and there are other installation methods available. GitLab.com itself is a large Enterprise Edition instance. This has some implications:
A merge request may benefit from being considered a customer critical priority because there is a significant benefit to the business in doing so.
Properties of customer critical merge requests:
customer-critical-merge-request label to the merge request.There are some cases where pipelines fail for reasons unrelated to the code changes that have been made. Some of these cases are listed here with a potential solution.
Always remember that you don't need to have a passing pipeline in order to ask for a review, or help.
If your pipeline is not passing and you have no idea why, feel free to reach out to the team, ask for help from MR coaches by leaving a comment on the MR with @gitlab-bot help as text, or reach out to the Community Discord in the contribute channel.
danger-review job failed: check if your MR has more that 20 commits. If that's the case, rebase and squash them to have less commits, otherwise try to run the danger-review job again, it might just have been a temporary failure.How code reviews are conducted can surprise new contributors. Here are some examples of code reviews that should help to orient you as to what to expect.
"Modify DiffNote to reuse it for Designs":
It contained everything from nitpicks around newlines to reasoning
about what versions for designs are, how we should compare them
if there was no previous version of a certain file (parent vs.
blank sha vs empty tree).
"Support multi-line suggestions": The MR itself consists of a collaboration between FE and BE, and documenting comments from the author for the reviewer. There's some nitpicks, some questions for information, and towards the end, a security vulnerability.
"Allow multiple repositories per project":
ZJ referred to the other projects (workhorse) this might impact,
suggested some improvements for consistency. And James' comments
helped us with overall code quality (using delegation, &. those
types of things), and making the code more robust.
"Support multiple assignees for merge requests": A good example of collaboration on an MR touching multiple parts of the codebase. Nick pointed out interesting edge cases, James Lopez also joined in raising concerns on import/export feature.
Largely based on the thoughtbot code review guide.