Transition from maintainers to "code standard" approvers

🔭 Problems this attempts to address

  • Increase the total number of maintainers and create a path to more evenly distributed review responsibilities.
  • More efficient reviews, most specifically for those who transition to Code Standard reviewers by making them more focused.
  • Make becoming a Code Standard reviewer more approachable.
  • Change the narrative of how maintainership is sometimes viewed to being a "diverse many" rather than a "focused few".
  • Remove ambiguity around who the domain expert reviewer is or should be for a given review.
  • Create a process that can continue to scale.

🗺 Overview

In the Maintainership working group, one idea that has been mentioned once or twice in sync meetings is that in previous roles, it was assumed that Senior engineers had merge rights (Maintainer permissions role). Some were surprised that this was not the case at GitLab.

I found myself thinking about this and asking "What if we updated all senior engineers to Maintainer roles on their respective projects?". How could that work? What would be the consequences? What risk would be added?

I came to a few conclusions:

  1. I don't believe there would be abuse. We trust that our Senior engineers are looking out for the code.
  2. I don't believe Seniors would start merging code they are not comfortable merging.

Point (2.) got me thinking about what code I personally am comfortable merging. I am comfortable with code that is:

  • In my domain
  • In areas or topics where I have large experience or expertise

In the gitlab-org/gitlab project (I'll refer to it as the rails project), we have a few different reviewer roles such as frontend/backend/database/etc.... On the security repo, we also include the appsec/security reviewer. These are all covered in the approval guidelines

If a given MR requires more than one of those reviews, the maintainers know not to merge it until it has all of the approvals. There is nothing stopping any of them from merging prior to that, but that doesn't happen because we all trust the process and each other.

This leads to a few facts:

  1. Maintainers are expected to have "expertise in one or more domains", but more uniquely, "a deep understanding of our coding standards".
  2. We advocate that one reviewer should be a domain expert if possible. But it is ambiguous as to if this should be the initial reviewer or the maintainer reviewer. Sometimes the maintainer acts as both.

💡 Proposal

Given the unique aspect of a maintainer is their "understanding of our coding standards", what if we made maintainers Code Standard approvers in their perspective areas of expertise with the ability to approve an MR from the perspective of code quality and guidelines?

In a simple backend MR, for example, instead of having an "initial review" and "maintainer review", we require a "domain review approval" and "code standard approval".

🧪 How this could work

  • All existing maintainers are re-defined as Code Standard reviewers.
  • All Senior+ engineers are given Maintainer role on their projects and are considered Domain reviewers.
    • Note: this means that many maintainers will now have dual Maintainership as both Code Standard reviewers and Domain reviewers in their respective areas.
  • Intermediate engineers can still gain Maintainer roles as a Domain reviewer, Code Standard reviewer, or so on through a modified system similar to our existing trainee system.
  • Once all approvals exist on a given MR, any approver that is a Maintainer can merge the MR (this is essentially already true, we just expand who can be a Maintainer here).

⤴ Benefits

  • By narrowing the scope of existing maintainer reviews to Code Standards, existing maintainers now have a reduction in responsibility, which could lead to faster reviews.

  • By moving towards a Code Standard reviewer, the smaller scope may lead to more engineers having interest in pursuing that responsibility.

  • By allowing Domain experts to have merge rights, there are more people who are capable of merging once all of the approvals are there.

  • We place more trust in Senior engineers with less scrutiny.

  • The process can scale to easily include approvals for other topics. For example, if we decided GraphQL is enough of a specialty that MRs with GraphQL changes should have a GraphQL expert, then we could create a GraphQL Maintainer approval category.

⤵ Cons

  • There may be cases where an extra review is needed when there is not a Maintainer available for a given Domain.
  • Probably more here, but I'm having a hard time putting them together as I write this.

📝 Some questions

What else do we need to know?

If this idea is pursued, we would need to understand what the maintainer and approver distribution would look like after the initial change to make existing Senior+ engineers Maintainers. It is possible it would create deficits of reviewers in other areas, recreating the same problem we have now.

What about other projects?

I thought of this mostly thinking about the rails project, but I think it could still work for smaller projects. We need one person who knows the code (the Domain approver) and one person who knows the guidelines (the Code Standard approver). I'm not sure what the numbers look like on other projects, but this change would at least increase the Domain approvers on other projects by promoting all Senior+ engineers to Maintainer roles on those projects. And similar as explained above, the Code Standard approver would have less burden, so there could potentially be fewer of them.

This seems like it would be a massive change, how does it help right now?

It is true, this is a large process change that would effect all of engineering and would not be quick to implement because we have so much built around the existing process. That is why I've presented it first in an issue since the implementation would likely require many MRs and a rollout plan.

👋 What I'd like from you

  • What do you think of the idea?
  • What are the holes you see?
  • How does it compare to our existing system?
  • If you are in a Senior role and not a Maintainer on your project, how do you feel about this?
  • If you are a Maintainer, how do you feel about your that title being redefined?
  • If this makes you think of another idea or altered version of this idea, please do share it!
Edited by Steve Abrams