Skip to content

Latest commit

 

History

History
184 lines (136 loc) · 7.01 KB

p1367.md

File metadata and controls

184 lines (136 loc) · 7.01 KB
Raw

Remove CODEOWNERS

Pull request

Table of contents

Problem

We want to restrict who can merge files, in particular to ensure reviews are done.

Currently we use CODEOWNERS for this purpose. However, CODEOWNERS can lead to review bottlenecks, creating possibly unnecessary hindrances to development, as discussed in the background.

Background

Current CODEOWNERS

We currently use, in brief:

*                   @carbon-language/implementation-team
/*                  @carbon-language/implementation-team
/*.md               @carbon-language/carbon-leads @carbon-language/implementation-team
/docs/              @carbon-language/carbon-leads @carbon-language/implementation-team
/proposals/         @carbon-language/carbon-leads
/LICENSE            @carbon-language/carbon-leads
CODEOWNERS          @carbon-language/carbon-leads

CODEOWNERS issues

Issue #413: Consider removing CODEOWNERS -- at least for now was used to discuss this issue. This can lead to a few issues:

  • CODEOWNERS defaults to assignment to the group, which can lead to a "tragedy of the commons" situation where everybody expects someone else to review.
    • In assigning to the group, CODEOWNERS also makes for noisy PR notifications.
    • Note both of these issues can be addressed with GitHub review settings.
  • CODEOWNERS doesn't accurately handle the difference between a "major" change and a "minor" change.
    • For example, a new proposal should only be approved by carbon-leads, while a typo fix to a proposal would be fine for anyone to approve.
    • Decisions to make minor proposal edits more common, as in Issue #1009: Should doc edits maintain proposal links?, exacerbate this issue.

Proposal

Remove CODEOWNERS, and instead use commit access to gate PRs.

Details

Group access controls

  • Base: read (unchanged)
    • Should be the standard public permission.
  • Contributors: "contributor" (previously write)
    • The "Contributors" group should be easy to get access to. On top of read, this should include the ability to add/remove labels and projects (may require some work to get right).
    • Versus "triage", "triage" seems to have powers including deleting conversations, etc, that may not make sense for this group.
  • Implementation team: write (unchanged)
    • For people expected to be reviewing commits.
  • Moderators: triage
  • Leads: maintain (unchanged)
  • Admin team: admin (unchanged)

For people who have access, settings are at:

rm CODEOWNERS

This should only be done after group access controls are updated. Once done, group access controls are the last word on who can commit PRs.

Auto-assignment

This PR introduces auto-assignment in order to ensure PRs aren't lost. It provides categories of assignment, and a fallback for other PRs that don't have explicit assignment.

Stacked PRs

Advice for stacked PRs is no longer "just ask an admin"; it is "commit access required". This is because removing CODEOWNERS removes boundary between merge access to a feature branch and merge access to trunk.

Rationale

Alternatives considered

Fine-grained CODEOWNERS

We could keep the current fine-grained CODEOWNERS. In this setup, we would try to have specific directories owned by specific teams.

We've ended up on this path so far so that we have some mechanical enforcement of who's reviewing. We may need to change up the groups a little, but a reasonable long-term expectation would be something like a docs team, an explorer team, a toolchain team, etc, with each owning their own files. For example, /docs is owned only by the docs team, and there are people in the docs team who aren't in other teams (and the other way around).

As previously mentioned, if we were using this option, we may want to change GitHub review settings in order to fix email/assignment.

Advantages:

  • Relies on built-in GitHub CODEOWNERS featureset, intended for this purpose.
    • Can use built-in features for review assignment.
  • Works with current stacked PR advice.

Disadvantages:

  • Retains team-specific bottlenecks: if none of a directory's owners are available, other contributors cannot step in.
    • This could also be considered to be an advantage by some.
  • GitHub's built-in features for review assignment are not as feature-rich as some custom tooling.

It's mainly because of the relative inflexibility of CODEOWNERS that we aren't doing this.

Broaden CODEOWNERS

We could broaden the CODEOWNERS to something basic, like a single group owning everything. Reviews in a particular area become best practice, not mechanically enforced.

Advantages:

  • Simple to understand.
    • Avoids custom tooling.

Disadvantages:

  • We expect multiple subprojects within the carbon-lang repository, and this approach would make it difficult to determine when to review a PR.
    • A large group means GitHub's built-in auto-assignment would assign to arbitrary CODEOWNERS.
    • The notification issues couldn't be addressed.

This option is likely worse for our purposes than fine-grained CODEOWNERS.