Discussion: whether to recommend assigning a maintainer from a different team than your own, or not
I spoke about this briefly with @lbennett on Slack a few weeks ago:
Luke Bennett: Just realised we don't have a single maintainer under Manage, I know we're not exclusively working in teams, but I think it would make sense, I see some people here that could definitely be maintainers but probably aren't for the sake of their availability and sanity.
😛 Douwe Maan: I was thinking about this at the summit, and I think we should treat maintainers as a group totally separate from teams. Even if a team has a maintainer, I’d prefer for MRs to be reviewed by maintainers from other teams, so that we ensure that code looks the same across the board, and that code remains understandable to everyone, including people in other teams, and outside contributors. If code is written by people with a full understanding of the feature and existing code, and reviewed by people with a full understanding of the feature and existing code, that makes it easier for code to slip in that is actually hard to understand to anyone outside of the silo. Teams may also develop their own code styles and patterns, which we should avoid. (edited)
He then created https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21644 to add the following to the Code Review documentation:
- It is recommended that you assign a maintainer that is from a different team than your own. This ensures that all code across GitLab is consistent and can be easily understood by all contributors.
I merged it thinking it was a relatively uncontroversial recommendation, but when I brought it up on the Backend weekly call agenda earlier today, @grzesiek and others brought up some very good points that made it less straightforward than I had thought:
Grzegorz: Isn’t that going to lower quality of features that we implement?
Douwe: The point isn’t that maintainers from the team are not allowed to see the code, but that the code should ultimately be signed off by someone who isn’t already an expert on this part of the code base. Maintainers and other team members should definitely be involved in the design of the implementation and the initial rounds of review.
Grzegorz: I understand the intent here, but because maintainers are really busy they usually won’t review merge requests that are not assigned to them, this way domain experts won’t review some merge requests.
Douwe: In my opinion (which is not stated policy, and should definitely be challenged), maintainer review should be more about the things that are less product area specific, like overall architecture, code organization, separation of concerns, etc. It’s not the right time to find edge cases etc, those should be found in earlier stages, during development, or in review by team members/domain experts
Grzegorz: I agree with that as well, but this is not how code review works at GitLab, at the moment :) .
Rachel: This places a dependency on someone outside of the team to ship a feature. Different teams may have different priorities and it could be that your team’s change is not as high a priority and does not get addressed quickly enough.
We discussed it some more in the call itself, and I invite everyone to elaborate on and share their perspectives here.
I've reverted https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/21644 for the time being, since the current recommendation is untenable as long as it is not well defined and understood what is actually expected from a Maintainer: whether they are responsible for ensuring the overall quality of what an MR means to implement, which requires them to be a domain expert, or whether they should just ensure of the quality of the code itself which should be consistent across the codebase, independent of domain/product area.
/cc @ayufan @dbalexandre @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern
/cc @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann