Gitlab hero border pattern left svg Gitlab hero border pattern right svg

Code Review Guidelines

  1. You are here:
  2. Engineering
  3. Engineering Workflow
  4. Code Review Guidelines

Overview

Code reviews are mandatory for every merge request, you should get familiar with and follow our Code Review Guidelines.

These guidelines also describe who would need to review, approve and merge your, or a community member's, merge request. They also describe the review response time SLO's team members have to abide by.

Values

Every reviewer at GitLab must strive for our reviewer values.

Reviewer

All GitLab engineers can, and are encouraged to, perform a code review on merge requests of colleagues and community contributors. If you want to review merge requests, you can wait until someone assigns you one, but you are also more than welcome to browse the list of open merge requests and leave any feedback or questions you may have.

You can find someone to review your merge requests by looking on the team page, or on the list of GitLab Engineering Projects, both of which are fed by YAML files under data/team_members/person/*.

You can also help community contributors get their merge requests ready, by becoming a Merge Request Coach.

Note that while all engineers can review all merge requests, the ability to accept merge requests is restricted to maintainers.

Maintainer

Maintainers are GitLab engineers who are experts at code review, know the GitLab product and codebase very well, and are empowered to accept merge requests in one or several GitLab Engineering Projects.

Every project should have at least two maintainers, but most should have more. Some projects have separate maintainers for different specialties. For example, the main GitLab codebase has separate maintainers for frontend, backend, and database.

We do not expect maintainers to know every single part of such a massive project. That expectation simply doesn't scale with an organization of our size. As such we expect our reviewers and maintainers to know their limits and pass to or request additional reviews from domain experts if they feel they don't fully understand the code in question.

Becoming a reviewer/maintainer means you are taking on a broader responsibility beyond your immediate group. Your available capacity should be adjusted accordingly to make room for you to work effectively. There is no strict formula for this as each project comes with a different workload, but do make sure to discuss this with your manager to avoid burnout and to ensure your manager understands how this may impact your team's capacity.

To protect and ensure the quality of the codebase and the product as a whole, people become maintainers only once they have convincingly demonstrated that their reviewing skills are at a comparable level to those of existing maintainers.

As with regular reviewers, maintainers can be found on the team page, or on the list of GitLab Engineering Projects.

Senior+ Maintainers

For senior+ engineers, being a maintainer is part of their job family unless they have discussed with their manager or Team Member Relations in line with our reasonable accommodation process. Effective 2022-08-01, we use the following table for the timeframe expectations of maintainership:

Description Timeframe
Intermediate Engineers Maintainership is optional
Existing Senior+ Engineer Existing senior+ engineers who are not already maintainers are encouraged to complete the trainee program in support of our team's productivity and motivation. There is no expected completion timeframe of the trainee program.
Newly hired Senior+ During onboarding, newly hired senior+ engineers will be asked to become trainee maintainers instead of reviewers. We expect their maintainership to be complete within 12 months of their onboarding completion.
Promotions to Senior For engineers moving into the Senior role, we expect that they have already become a maintainer prior to promotion.

Meeting the reviewer/maintainer

Communication happens easier when you are familiar with the person reviewing the code. Take opportunities (for example coffee chats) to get to know reviewers to break the ice and facilitate future communication.

How to become a project maintainer

This applies specifically to backend, frontend and database maintainers. Other areas (docs, etc.) may have separate processes.

As a reviewer, a great way to improve your reviewing skills is to participate in MRs. Add your review notes, pass them on to maintainers, and follow the conversation until the MR is closed. If a comment doesn't make sense to you, ask the commenter to explain further. If you missed something in your review, figure out why you didn't see it, and note it down for next time.

We have two guidelines for project maintainership, but no concrete rules:

  1. In general, the further along in their career someone is, the more we expect them to be capable of becoming a maintainer.
  2. Maintainers should have an advanced understanding of the GitLab project that they wish to maintain. Before applying for maintainership, a person should get a good feel for the project codebase, expertise in one or more domains, and deep understanding of our coding standards.

Apart from that, someone can be considered as a project maintainer when both:

  1. The MRs they've reviewed consistently make it through maintainer review without significant additionally required changes.
  2. The MRs they've written consistently make it through reviewer and maintainer review without significant required changes.

Once those are done, they should:

  1. Create a MR to add the maintainership for the project they wish to maintain to their team page entry.
  2. Explain in the MR body why they are ready to take on that responsibility.
  3. Use specific examples of recent "maintainer-level" reviews that they have performed. These include but are not limited to:
    1. MRs that introduce non-trivial changes (architectural changes, refactors, developer experience upgrades)
    2. MRs that improve our quality of the project (tests, performance)
    3. MRs that deliver full features (ideally this is done iteratively, so a good maintainer can guide someone to break down large MRs to smaller ones)
    4. Note: MRs that introduce very simple changes are good, but should not be the only source of reviews
  4. Assign the MR to their manager and mention the existing maintainers of the relevant project (GitLab, GitLab Shell, etc) and area (backend, frontend, etc.).

The approval process to follow is:

  1. The MR will remain open for 5 working days.
    1. For a maintainer category that consists of 10 or fewer maintainers, if more than half of the existing maintainers of the relevant engineering group (e.g. backend) approve the MR, and there are no blocking concerns raised during this period; we've got a new maintainer!
    2. For a maintainer category that consists of 11 or more maintainers, if 5 existing maintainers of the relevant engineering group (e.g. backend) approve the MR, and there are no blocking concerns raised during this period; we've got a new maintainer!
    3. Maintainers should consider being an approver only if they can support the approval with multiple examples of the trainee's skills, likely from MRs they reviewed together.
  2. If the MR does not have enough approvals after this period, and no blocking concerns, then the manager should mention the existing maintainers again in a new comment as well as in the relevant Slack channel (e.g. #backend) and keep the MR open for 5 more working days.
  3. After the extra period, any missing feedback from a maintainer should be counted as a neutral response and the manager can merge the MR given no blocking concerns were raised!
  4. Normal non-blocking feedback should be submitted as comments to the MR. To support Gitlab Values and avoid groupthink, any blocking negative feedback should be instead submitted to the manager of the aspiring maintainer. The manager will then share the feedback with the aspiring maintainer and post a generic message with a "TL;DR" in the MR.
  5. If there are blocking concerns raised at any point in the approval process, the maintainers who raised the concern should actively work with the maintainer nominee's manager to develop a plan on how to resolve the concern.

Since the manager of the new maintainer is the MR assignee, they should be the one merging the MR.

It is helpful if the person merging the MR also leaves a comment with a summary, see example 1 or example 2 for reference.

When the manager merges the MR, they should announce this change in the applicable channels listed under keeping yourself informed section of the engineering handbook and #backend_maintainers/#frontend_maintainers and #backend/#frontend. We should also update the Engineering Week-in-Review document. The agenda is internal only, please search in Google Drive for 'Engineering Week-in-Review'.

After becoming a maintainer

If you've become a new maintainer, follow these instructions to request relevant permissions that will allow you to fulfill your role:

Trainee maintainer

To help grow the maintainer base with the team, we allow for 'trainee maintainers'. These are reviewers who have shown a specific interest in becoming a maintainer, and are actively working towards that goal.

Anyone may nominate themselves as a trainee by opening a tracking issue using the Trainee backend maintainer template, Trainee frontend maintainer template, Trainee database maintainer template, or Trainee quality maintainer template. It's normally a good idea to check with at least one maintainer or your manager before creating the issue, but it's not required.

After opening your tracking issue, create a Merge Request for the team page proposing yourself as a trainee maintainer.

Most backend trainees, working full-time without significant interruptions (for example, parental leave) reach the point where they are ready to become a maintainer in five to seven months. If it takes longer, that's OK.

Trainees should feel free to discuss process or progress with their manager or any maintainer, at any time. We recommend that the managers of trainee maintainers arrange a check-in every six weeks or so during the process, to assess where they are and what remains to be done.

If you'd like to work towards becoming a maintainer, discuss it in your regular 1:1 meetings with your manager. They will help you to identify areas to work on before following the process above.

Trainee maintainer mentorship pilot program

Training and onboarding new maintainers is an important process. As the engineering team grows and the total number of MRs rapidly expands, the number of MR reviews per maintainer quickly becomes unsustainable.

Recent research has shown that the trainee process for new maintainers is hindered by several key factors:

Structure

  1. Participation is voluntary for both Maintainers and Trainees.
  2. Maintainers may directly mentor up to 4 Trainees at a time.
  3. Mentor / Trainee assignments are coordinated within a maintainer_mentorship.yml file.
  4. New Trainees need to locate an existing Maintainer that has an opening available in maintainer_mentorship.yml and contact the Maintainer directly.
  5. Trainees and mentors can join the #trainee_maintainers Slack channel to discuss the program or ask questions.
  6. Every 6 weeks the Maintainer will check-in with each Trainee. This could happen async or via Coffee chat.
  7. The goal of the checkin is to: review MRs, answer questions, clarify any doubts, and track readiness toward graduating.
  8. Mentorship is capped at 12 months by which the Trainee should be prepared to graduate.
  9. At least 1 additional check-in should be scheduled once the Trainee has graduated to celebrate the achievement and answer any further questions.
  10. Make sure to remove the graduated Trainee to make space for new Trainees.

Benefits

  1. It develops and expands mentorship skills among maintainers
  2. Gives trainees a regular touch point for skilling-up in deficient areas
  3. Creates stronger networks amongst trainees/maintainers than currently exists
  4. Mentoring directly under a maintainer should catalyze more competence and confidence in taking ownership of the GitLab codebase.

After becoming a maintainer

If you’ve become a new maintainer, follow these instructions to request relevant permissions that will allow you to fulfill your role:

Transitioning away from being a reviewer/maintainer

After consultation with your manager, you may wish or need to transition away from being a reviewer/maintainer. Regardless of the circumstances, it's perfectly OK for this to happen! Responsibilities and workloads change; projects evolve. So it's important to ensure your time is spent on the areas that are most important. To make the change official and to be removed from reviewer roulette:

  1. See the Team Member Database document for how to update your YAML file.
  2. Create a new MR and assign the MR to your manager.
Returning to become a reviewer/maintainer

After a period of transitioning away from being a reviewer/maintainer, you may wish to return to performing these duties. To make the request official, see the section on becoming a Trainee maintainer, creating the tracking issue and Merge Request, referencing previous tracking issue(s) and Merge Request(s) for context. The newly created Merge Request should be assigned to your manager for immediate review as the process can usually be fast tracked.

Maintainership process for smaller projects

This is a general purpose template that projects belonging to a specific group and/or projects with less than 10 internal contributors can use to define and document their maintainership process.

Projects may adopt these guidelines for maintainership to help grow maintainers in projects where there are not enough maintainers.

Trainee maintainer issue template

Use this lightweight template as a starting point for creating your project's trainee maintainer issue template.

## Basic Setup

Thank you for becoming a [Project] maintainer trainee!
Please work on the list below to complete your setup.

- [ ] Change issue title to include your name: `[Project] Trainee Maintainer: [Full Name]`.
- [ ] Complete project-specific language training if the programming language is not used in the main GitLab project (example: [golang training](https://gitlab.com/gitlab-com/www-gitlab-com/-/blob/master/.gitlab/issue_templates/golang_training.md)).
- [ ] Review general [code review guidelines](https://docs.gitlab.com/ee/development/code_review.html).
- [ ] Review project-specific release process (if one exists).
- [ ] Join the `[project or team]` Slack channel.
- [ ] Create a merge request and indicate your role as a `project-name: reviewer` or `project-name: trainee_maintainer` in your [team member entry](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/doc/team_database.md). Assign MR to your manager for merge.
- [ ] _Optional:_ Perform simulated reviews on practice MRs if available. Add a link to the MR and your review as a discussion in this issue.
- [ ] _Optional:_ Peer review MRs. Add a link to the MR and your review as a discussion in this issue.
- [ ] _Optional:_ Consider working on issues to gain familiarity with the project's domain and guidelines.
- [ ] _Optional:_ Reach out to an existing maintainer to [help you become](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer-mentorship-pilot-program) a maintainer.

## Working towards becoming a maintainer

There is no checklist here, only guidelines. Remember that there is no specific timeline on this.
In the case where you feel you are lacking more "complex" reviews, consider yourself ready enough.

Your reviews should aim to cover maintainer responsibilities as well as reviewer
responsibilities. Your approval means you think it is ready to merge.

After each MR is merged or closed, add a discussion to this issue using this
template:

<details><summary>MR discussion template</summary>

  ### (Merge request title): (Merge request URL)

  During review:

  - (List anything of note, or a quick summary. "I suggested/identified/noted...")

  Post-review:

  - (List anything of note, or a quick summary. "I missed..." or "Merged as-is")

  (Maintainer who reviewed this merge request) Please add feedback, and compare
  this review to the average maintainer review.
  
</details>

**Note:** Do not include reviews of security MRs because review feedback might
reveal security issue details.

**Tip:** There are [tools](https://about.gitlab.com/handbook/tools-and-tips/#trainee-maintainer-issue-upkeep) available to assist with this task.

## When you're ready to make it official

When reviews have accumulated, and recent reviews consistently fulfill
maintainer responsibilities, any trainee can take the next step. The trainee
should also feel free to discuss their progress with their manager or any
maintainer at any time.

1. [ ] Create a merge request updating [your team member entry](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/doc/team_database.md) proposing yourself as a maintainer and link to this training issue. Assign to your manager.
2. [ ] Get yourself added as a Maintainer to `[project]`. Reach out to any existing maintainer.
3. [ ] Keep reviewing, start merging :metal:

/label ~"trainee maintainer"

Maintainer ratios

We aim to keep the engineer : maintainer ratio under 6, for both frontend and backend. We track this in the Engineer : Maintainer Ratio dashboard:

Maintainer load

We aim to keep the merge request per maintainer type at a reasonable level as well. We track this in the Merge Requests: MR Count and Ratio by FE/BE/DB:

Resident Contributor

All wider community members can, and are encouraged to, contribute in frequent and atomic iterations. We encourage and celebrate those that contribute on a regular basis. As soon as a wider community member reaches 2 merged MR's on average over 3 months, we consider that person a Resident Contributor and they are entitled to a review response SLO among other benefits. These statuses are updated after each monthly release. Merge requests that were created when the author was a Resident Contributor will keep the SLO commitment.

Resident Contributor = (total merged MRs over 90 days) / 3 >= 2

Domain Experts

Our Code Review Guidelines states that we default to assigning reviews to team members with domain expertise.

What makes a domain expert?

We currently don't provide rigid rules for what qualifies a team member as a domain expert and instead we use a boring solution of implicit and self-identification.

Implicit:

Self-identification:

How to self-identify as a domain expert

The only requirement to be considered a domain expert is to have substantial experience with a specific technology, product feature or area of the codebase. We leave it up to the team member to decide whether they meet this criteria.

  1. Define a new, or use an existing domain expertise key, located in data/domain_expertise.yml.
  2. Update your entry in your own YAML file with a new domain_expertise property and list all domain expertise keys.

Example:

domain_expertise.yml

webpack:
  display_name: Webpack
  link: https://webpack.js.org/
frontend_architecture:
  display_name: Frontend Architecture
  link: https://docs.gitlab.com/ee/development/fe_guide/architecture.html

your_handle.yml

domain_expertise:
    - webpack
    - frontend_architecture

When self-identifying as a domain expert, it is recommended to assign the MR to be merged by an already established Domain Expert or a corresponding Engineering Manager.

Where can I find a list of people with domain expertise?

The expertise of a team member can be seen on the Engineering Projects page.

Review turnaround time

Because unblocking others is always a top priority, reviewers are expected to review merge requests in a timely manner, even when this may negatively impact their other tasks and priorities.

Doing so allows everyone involved in the merge request to iterate faster as the context is fresh in memory, and improves contributors' experience significantly.

Review-response SLO

To ensure swift feedback to ready-to-review code, we maintain a Review-response Service-level Objective (SLO). The SLO applies to GitLab team members and resident contributors, but not to other wider community contributors.

The SLO is defined as:

Review-response SLO = (time when review is provided) - (time MR is assigned to reviewer)

The SLO value depends on the author of the merge request:

If you don't think you can review a merge request in the Review-response SLO time frame, let the author know as soon as possible in the comments (no later than 36 hours after first receiving the review request) and try to help them find another reviewer or maintainer who is able to, so that they can be unblocked and get on with their work quickly. Remove yourself as a reviewer.

If you are at capacity and are unable to accept any more reviews until some have been completed, communicate this through your GitLab status by setting the 🔴 :red_circle: emoji and mentioning that you are at capacity in the status text. This guides contributors to pick a different reviewer, helping us to meet the SLO.

Of course, if you are out of office and have communicated this through your GitLab.com Status, authors are expected to realize this and find a different reviewer themselves.

When a merge request author has been blocked for longer than the Review-response SLO, they are free to remind the reviewer through Slack or add another reviewer.

Managing expectation

When you are assigned to review an MR and you are not able to get to it within the Review-response SLO, you should leave a comment on the MR informing the author of your delayed response. If possible, you should also indicate when the author can expect your feedback or help them find an alternative reviewer.

As the author of an MR you should reassign to another reviewer or maintainer if the Review-response SLO has not been met and you have been unable to contact the assignee.

Open in Web IDE View source