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.
Every reviewer at GitLab must strive for our reviewer values.
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.
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.
Great engineers are often also great reviewers, but code review is a skill in and of itself and not every engineer, no matter their seniority, will have had the same opportunities to hone that skill. It's also important to note that a big part of being a good maintainer comes from knowing the existing product and codebase extremely well, which lets them spot inconsistencies, edge cases, or non-obvious interactions with other features that would otherwise be missed easily.
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.
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. |
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.
This applies specifically to backend, frontend and database maintainers. Other areas (docs, etc.) may have separate processes.
Before considering maintainership, first you should be a contributor. You should have made at least a few feature or maintenance contributions to the project before you can become a reviewer in the trainee maintainer process. These contributions should be complex enough to give you an understanding of the project's unique domain and design.
Interested reviewers should check in regularly with their manager to discuss progress towards maintainership and review any recent detailed reviews, for example during their 1-on-1s. Reviewers are encouraged to also seek out a maintainer mentor for further perspective on their reviews. Reviewers are encouraged to think of their eligibility for maintainership in the terms of "I could be ready at any time to be a maintainer as long as it is justified".
After each review is complete, the reviewer should write up a justification about why they believe the merge request is ready to merge. This justification is then reviewed by the maintainer and if the maintainer agrees with the justification they should add a đź‘Ť reaction to the comment, even if they have additional non-blocking comments. The maintainer should leave a comment highlighting any blocking concerns that were missed in the initial review.
At any time, the manager/mentor may choose to open a merge request, adding the reviewer as a maintainer. This merge request should have a justification from the manager as to why the reviewer should become a maintainer. You are also welcome to open this merge request yourself at any time.
Before opening the merge request, the reviewer should:
Before merging, the manager should:
If the manager is given feedback that indicates the reviewer is not ready to become a maintainer: the manager should close the merge request and provide the feedback directly to the reviewer to address the gaps before the reviewer is re-submitted. The earlier the manager can solicit this feedback the better.
#backend_maintainers/#frontend_maintainers and #backend/#frontend.If you've become a new maintainer, follow these instructions to request relevant permissions that will allow you to fulfill your role:
#frontend_maintainers, #backend_maintainers, #database_maintainers etc.While any reviewer may be recommended by their manager to become a maintainer at any time, reviewers who wish to become maintainers should follow a few basic steps on each review in order to get into a maintainer mindset, and learn from feedback from maintainers.
Create a merge request and indicate your role as a project-name: reviewer or project-name: trainee_maintainer in your team member entry. Assign MR to your manager for merge.
After each review, reviewers should summarize why they believe a merge request is ready to be merged:
For example:
Looks good! I believe this MR resolves the issue and it looks safe because the code change is relatively isolated.
LGTM! I feel this MR is a good iteration. And it has low risk because it is behind a feature flag.
Maintainers should respond to the comment from the reviewer with a đź‘Ť if they agree, and upon merging if there were additional comments they feel should have been caught, they should ping any reviewers so they are aware of the comments.
Some reviewers find it helpful to track their progress. This is not required, but a few ways people have done this are:
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:
#trainee_maintainers Slack channel to discuss the program or ask questions.If you’ve become a new maintainer, follow these instructions to request relevant permissions that will allow you to fulfill your role:
#frontend_maintainers, #backend_maintainers, etc.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:
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.
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.
simple_roulette within the project to identify MR reviewers.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]`.
- [ ] Work on issues to gain familiarity with the project's domain and guidelines.
- [ ] 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:_ 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"
We aim to keep the engineer : maintainer ratio under 6, for both frontend and backend. We track this in the Engineer : Maintainer Ratio dashboard:
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:
All wider community members, and their organizations, can
contribute. We strongly believe that contributing to
open source, and particularly to GitLab is a competitive advantage for organizations and their members, and want to
reward those organizations for doing so. GitLab highly encourages, celebrates & rewards those that contribute in
frequent and atomic iterations. When an organization or individual without affiliations
reaches 20 merged merge requests or more within the last 3 completed months, we consider that organization or individual
a Leading Organization.
Organizations are matched based on the Organization field in your profile. GitLab
can also match individuals to organizations using other metadata available to the Contributor Success team. Create an
issue in the Contributor Success queue if you
think you or your organization should qualify but is not receiving the label Leading Organization on your merge
requests.
A Leading Organization is entitled to a review response SLO. This entitlement will be
awarded in the beginning of each month. Merge requests that were created during the time in which an organization is
entitled to the Leading Organization status, will receive the label Leading Organization.
Leading Organization = 20 merged Merge Requests or more within the last 3 completed months.
Eligible merge requests include contributions to the GitLab product and documentation. Contributions to the www-gitlab-com repository (e.g. the GitLab handbook) are not currently included or entitled to a review response SLO.
Our Code Review Guidelines states that we default to assigning reviews to team members with domain expertise.
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:
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.
data/domain_expertise.yml.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.
The expertise of a team member can be seen on the Engineering Projects page.
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.
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 Leading Organizations, 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:
Review-response SLO < 2 business daysReview-response SLO < 4 business daysIf 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.
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.