Skip to content

WIP: Introduce domain maintainers into code review process

Jean du Plessis requested to merge domain_maintainers into master

What does this MR do?

Introduces the concept of domain maintainers into the code review process. For context, refer to gitlab-com/www-gitlab-com#4517.

Why this change

The GitLab codebase has grown in size and complexity to the point where it is very difficult for a single team member to have context and expertise across the full product.

In line with our objective of availability over velocity we need to ensure that solutions we implement are architecturally sound, performant and secure. To this extend we are introducing the concept of a domain maintainer.

This change aims to to achieve the following objectives:

  1. Improve the code quality from a solution point of view
  2. Contribute to code reviews being more efficient

By having team members with in-depth knowledge of a specific area of the codebase and product review all solutions in that area we aim to increase the quality and reduce the amount of time needed to conduct reviews.

By also removing the responsibility for reviewing the solution architecture from codebase maintainers, it allows them to have a more focused task which should further improve the code review process efficiency.

Proposal

This change introduces the concept of a domain maintainer into our code review process.

To differentiate domain maintainers from our current maintainers we will refer to them as codebase maintainers going forward.

Codebase maintainers are there to ensure that we write code across a single codebase in a coherent (what mechanisms we use) and cohesive (how we write code) way. Our codebase is growing larger and more complex by the day and codebase maintainers fulfill the critical function of ensuring we keep the barrier to contributing to the codebase low for new team members and community members.

Domain maintainers are primarily focused on validating the solution being implemented by ensuring that:

  1. it is accurate
  2. it is maintainable
  3. it is consistent with the architecture of the domain area
  4. it doesn't degrade our application performance or security

Segmenting by stage

Domain scope is defined by the stage that the feature/bug fix belongs to that is being implemented. For automation purposes (i.e. reviewer roulette), the stage can be determined by the devops::xxx label on the related issue for the MR. If the MR doesn't have an issue associated with it, it is up to the author to identify the stage and choose a relevant domain maintainer.

Code review process amendment

All Merge Requests must be approved by a domain maintainer before being assigned to a codebase maintainer.

The codebase maintainers will merge the code once all relevant approvals have been received.

The codebase maintainer will act as a gatekeeper for ensuring that a domain maintainer has signed off on a merge request before they merge it.

What happens to normal reviews?

Reviews from team members who are not a domain maintainer or codebase maintainer are encouraged, but are not mandatory for a merge request to be merged.

Becoming a domain maintainer

The objective is for all team members to be a domain maintainer in the stage they work in.

You can also be a domain maintainer and codebase maintainer at the same time, or only 1 of them, or none of them.

Eligibility criteria

  1. Should be a current team member of the stage (or group in the stage) they represent for at least 3 months.
  2. Must have a proven record of delivering and/or reviewing MRs in the stage they represent that shows they can fulfill the responsibilities of being a domain maintainer.

Nominating new domain maintainers

It is the responsibility of existing domain maintainers to recommend a team member to become a domain maintainer once the meet the eligibility criteria. Once a team member is recommended it requires the approval of two other domain maintainers (preferably from same stage) to accept the recommendation.

The DRI for ensuring a healthy domain maintainer to team member ratio lies with the Director of the stage. A healthy ratio is considered to be 1 domain maintainer for every 3 team members, with a minimum of 1 domain maintainer per group.

Defining the initial list of domain maintainers

The Development directors will be responsible for identifying and approving an initial list of team members to become domain maintainers.

Transition period

To accommodate the change in process and allow time to get all other related matters in place (see follow up actions) we should allow a transition period of 1 milestone. This means that we should only start enforcing this new process from Milestone 12.9 (2020-02-23) onwards.

Unresolved issues

  1. By introducing adomain maintainer review we are breaking the current trainee maintainer (to become trainee codebase maintainer) process. Currently trainee maintainers are 3x more likely to be picked by Reviewer Roulette to conduct reviews, however since a codebase maintainer does not qualify as a domain maintainer they will no longer be recommended for reviews by the Reviewer Roulette

Further Considerations

  1. Are the terms domain maintainer and codebase maintainer clear enough to comply with our MECEFU principle? Again we can debate an update this during the transition period, but should ideally be finalized before the rollout.
  2. Should we consider allowing domain maintainers to merge a MR that meets specific criteria themselves without the need for it to go through a codebase maintainer ? Possible examples are minor fixes to tests, copy, docs, or even small enough changes say less than n number of lines etc. This could also be addressed by a follow-up MR.
  3. Should we consider allowing team members who are both a domain maintainer and a codebase maintainer to merge after 1 review?
  4. Segmentation of the codebase by stage: is it feasible, or even preferable, to utilize the CODEOWNERS, functionality to identify areas of the codebase to a stage?
  5. Is segmentation by stage too big and should we rather segment by group?

Follow up actions

  1. Update all references to maintainers in the GitLab documentation and Handbook
  2. Update code review process in the handbook: https://about.gitlab.com/handbook/engineering/workflow/code-review/
  3. Development Directors should identify, in conjunction with engineering managers, the initial list of domain maintainers for the stages
  4. Modify reviewer roulette to make domain maintainer recommendation
  5. Add domain experts to Engineering Projects: https://about.gitlab.com/handbook/engineering/projects/
  6. Communicate the change to the company
Edited by Jean du Plessis

Merge request reports