Code reviews are mandatory for every merge request, you should get familiar with and follow our Code Review Guidelines. Because of the recognized criticality of building a community of contributors we put a high priority on ensuring community contributions receive a swift response to their submissions including a first-response SLO.
These guidelines also describe who would need to review, approve and merge your, or a community member's, merge request.
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 has at least one maintainer, but most have multiple. Some projects have separate maintainers for different specialties. For example, GitLab 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.
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.
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.
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:
Apart from that, someone can be considered as a project maintainer when both:
Once those are done, they should:
The approval process to follow is:
#backend) and keep the MR open for 5 more working days.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'.
The existing maintainers of the relevant engineering group and project will also raise any areas for growth on the merge request. If there are many gaps, the reviewer will need to address these before asking for reconsideration.
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.
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
#trainee_maintainers Slack channel to discuss the program or ask questions.Benefits
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.
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:
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.
To ensure swift feedback to ready-to-review code, we maintain a first-response Service-level Objective (SLO). The SLO is defined as:
- first-response SLO = (time when first response is provided) - (time MR is submitted and no longer marked WIP) < 2 business days
When you are assigned to review an MR and you are not able to get to it within the First-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 First-response SLO has not been met and you have been unable to contact the assignee.
