code_review.md

stage: none
group: unassigned
info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments

Code Review Guidelines

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.

Getting your merge request reviewed, approved, and merged

Before you begin:

• Familiarize yourself with the contribution acceptance criteria.
• If you need some guidance (for example, if it's your first merge request), feel free to ask one of the Merge request coaches.

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:

• Give you a second opinion on the chosen solution and implementation.
• Help look for bugs, logic problems, or uncovered edge cases.

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.

Read more about author responsibilities below.

Domain experts

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:

• Team members working in a specific stage/group (for example, create: source code) are considered domain experts for that area of the app they work on
• Team members working on a specific feature (for example, search) are considered domain experts for that feature

We default to assigning reviews to team members with domain expertise. When a suitable domain expert isn't available, you can choose any team member to review the MR, or simply follow the Reviewer roulette recommendation.

Team members' domain expertise can be viewed on the engineering projects page or on the GitLab team page.

Reviewer roulette

The Danger bot randomly picks a reviewer and a maintainer for each area of the codebase that your merge request seems to touch. It only makes recommendations and you should override it if you think someone else is a better fit!

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:
  • Contains the string OOO, PTO, Parental Leave, or Friends and Family.
  • GitLab user Busy indicator is set to True.
  • Emoji is from one of these categories:
    • On leave - 🌴 :palm_tree:, 🏖️ :beach:, ⛱️ :beach_umbrella:, 🏖️ :beach_with_umbrella:, 🌞 :sun_with_face:, 🎡 :ferris_wheel:
    • Out sick - 🌡️ :thermometer:, 🤒 :face_with_thermometer:
    • At capacity - 🔴 :red_circle:
    • Focus mode - 💡 :bulb: (focusing on their team's work)
• It 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:
  • 0️⃣ - :zero: (similar to :red_circle:)
  • 1️⃣ - :one:
  • 2️⃣ - :two:
  • 3️⃣ - :three:
  • 4️⃣ - :four:
  • 5️⃣ - :five:
• Team members whose Slack or GitLab status emoji is 🔵 :large_blue_circle: are more likely to be picked. This applies to both reviewers and trainee maintainers.
  • Reviewers with 🔵 :large_blue_circle: are two times as likely to be picked as other reviewers.
  • Trainee maintainers with 🔵 :large_blue_circle: are three times as likely to be picked as other reviewers.
• People whose GitLab status emoji is 🔶 :large_orange_diamond: or 🔸 :small_orange_diamond: are half as likely to be picked.
• 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.

The Roulette dashboard contains:

• Assignment events in the last 7 and 30 days.
• Currently assigned merge requests per person.
• Sorting by different criteria.
• A manual reviewer roulette.
• Local time information.

For more information, review the roulette README.

Approval guidelines

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.

1. If your merge request includes ~backend changes (1), it must be approved by a backend maintainer.
2. If your merge request includes database migrations or changes to expensive queries (2), it must be approved by a database maintainer. Read the database review guidelines for more details.
3. If your merge request includes ~frontend changes (1), it must be approved by a frontend maintainer.
4. If your merge request includes (~UX) user-facing changes (3), it must be approved by a Product Designer. See the design and user interface guidelines for details.
5. If your merge request includes adding a new JavaScript library (1)...
   • If the library significantly increases the bundle size, it must be approved by a frontend foundations member.
   • If the license used by the new library hasn't been approved for use in GitLab, the license must be approved by a legal department member. More information about license compatibility can be found in our GitLab Licensing and Compatibility documentation.
6. If your merge request includes a new dependency or a file system change, it must be approved by a Distribution team member. See how to work with the Distribution team for more details.
7. If your merge request includes documentation changes, it must be approved by a Technical writer, based on assignments in the appropriate DevOps stage group.
8. If your merge request includes changes to development guidelines, follow the review process and get the approvals accordingly.
9. If your merge request includes end-to-end and non-end-to-end changes (4), it must be approved by a Software Engineer in Test.
10. If your merge request only includes end-to-end changes (4) or if the MR author is a Software Engineer in Test, it must be approved by a Quality maintainer
11. If your merge request includes a new or updated application limit, it must be approved by a product manager.
12. If your merge request includes Product Intelligence (telemetry or analytics) changes, it should be reviewed and approved by a Product Intelligence engineer.
13. If your merge request includes an addition of, or changes to a Feature spec, it must be approved by a Quality maintainer or Quality reviewer.
14. If your merge request introduces a new service to GitLab (Puma, Sidekiq, Gitaly are examples), it must be approved by a product manager. See the process for adding a service component to GitLab for details.
15. If your merge request includes changes related to authentication or authorization, it must be approved by a Manage:Authentication and Authorization team member. 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 and Authorization section of the CODEOWNERS file, and the team will be listed in the approvers section of all merge requests that modify these files.

• (1): Specs other than JavaScript specs are considered ~backend code. Haml markup is considered ~frontend code. However, Ruby code within Haml templates is considered ~backend code.
• (2): 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.
• (3): 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.
• (4): End-to-end changes include all files within the qa directory.

Acceptance checklist

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.

Quality

See the test engineering process for further quality guidelines.

1. I have self-reviewed this MR per code review guidelines.
2. For the code that this change impacts, I believe that the automated tests (Testing Guide) validate functionality that is highly important to users (including consideration of all test levels).
3. If the existing automated tests do not cover the above functionality, I have added the necessary additional tests or added an issue to describe the automation testing gap and linked it to this MR.
4. I have considered the technical aspects of this change's impact on GitLab.com hosted customers and self-managed customers.
5. I have considered the impact of this change on the frontend, backend, and database portions of the system where appropriate and applied the ~ux, ~frontend, ~backend, and ~database labels accordingly.
6. I have tested this MR in all supported browsers, or determined that this testing is not needed.
7. I have confirmed that this change is backwards compatible across updates, or I have decided that this does not apply.
8. I have properly separated EE content from FOSS, or this MR is FOSS only.
   • Where should EE code go?
9. I have considered that existing data may be surprisingly varied. For example, a new model validation can break existing records. Consider making validation on existing data optional rather than required if you haven't confirmed that existing data will pass validation.

Performance, reliability, and availability

1. I am confident that this MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines)
2. I have added information for database reviewers in the MR description, or I have decided that it is unnecessary.
   • Does this MR have database-related changes?
3. I have considered the availability and reliability risks of this change.
4. I have considered the scalability risk based on future predicted growth.
5. I have considered the performance, reliability, and availability impacts of this change on large customers who may have significantly more data than the average customer.

Observability instrumentation

1. I have included enough instrumentation to facilitate debugging and proactive performance improvements through observability. See example of adding feature flags, logging, and instrumentation.

Documentation

1. I have included changelog trailers, or I have decided that they are not needed.
   • Does this MR need a changelog?
2. I have added/updated documentation or decided that documentation changes are unnecessary for this MR.
   • Is documentation required?

Security

1. I have confirmed that if this MR contains changes to processing or storing of credentials or tokens, authorization, and authentication methods, or other items described in the security review guidelines, I have added the ~security label and I have @-mentioned @gitlab-com/gl-security/appsec.
2. I have reviewed the documentation regarding internal application security reviews for when and how to request a security review and requested a security review if this is warranted for this change.

Deployment

1. I have considered using a feature flag for this change because the change may be high risk.
2. If I am using a feature flag, I plan to test the change in staging before I test it in production, and I have considered rolling it out to a subset of production customers before rolling it out to all customers.
   • When to use a feature flag
3. I have informed the Infrastructure department of a default setting or new setting change per definition of done, or decided that this is unnecessary.

The responsibility of the merge request author

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:

• It actually solves the problem it was meant to solve.
• It does so in the most appropriate way.
• It satisfies all requirements.
• There are no remaining bugs, logical problems, uncovered edge cases, or known vulnerabilities.

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 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:

• The addition of a linting rule (RuboCop, JS etc).
• The addition of a library (Ruby gem, JS lib etc).
• Where not obvious, a link to the parent class or method.
• Any benchmarking performed to complement the change.
• Potentially insecure code.

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.