nixpkgs branch protection rules: require status checks to pass

[wolfgangwalther]

NixOS/nixpkgs#417957 has the base to allow enabling required status checks efficiently, thus I propose to do that.

Requiring status checks would help prevent accidental merges. To be clear, they are not about lack of trust. Instead, they are about protecting committers. Nobody wants to break CI for others by accidentally merging something that actually fails CI. Also, those checks are a prerequisite for merge queues.

A little breakdown of how they work:

• The branch protection rule specifies the name of a status check. This will appear as "pending" in all PRs immediately after the rule is activated. It looks like this:

  

• Once a job with this name runs, it will replace that pending status check with either "success/skipped" or "failure". Skipped counts as success. When it's successful, a merge is allowed. When it's failed, the merge is blocked. In the UI, the merge button is disabled (similar to when a PR is merge conflicted). When merging from the command line, it shows like this:

   remote: error: GH013: Repository rule violations found for refs/heads/master.
   remote: Review all repository rules at https://github.com/wolfgangwalther/nixpkgs/rules?ref=refs%2Fheads%2Fmaster
   remote:
   remote: - Required status check "required to merge" is expected.
   remote:
   To github.com:wolfgangwalther/nixpkgs.git
    ! [remote rejected]           HEAD -> master (push declined due to repository rule violations)
   error: failed to push some refs to 'github.com:wolfgangwalther/nixpkgs.git'
  

• The branch protection rule can have a number of bypassers set up. The teams/roles with this privilege can merge a PR despite some required checks failing. In the web UI, it looks like this:

  

• When merging from the command line with bypass privileges, the push will always succeed. If a branch protection rule failed, it looks like this:

  remote: Bypassed rule violations for refs/heads/master:
  remote:
  remote: - Required status check "required to merge" is expected.
  remote:
  To github.com:wolfgangwalther/nixpkgs.git
     f68c81b1b9b4..a6086158c33b  HEAD -> master
  

Now, the question becomes: Which bypassers do we want to allow?

1. One approach could be to allow all committers to bypass, for example proposed by @Mic92 in teams/ci: init nixpkgs#416459 (comment).
   It's unfortunate that checks will always be bypassed from the command line - because merging from the command line is also in principle most likely to miss a failing check, since it's easier to see the list of checks right next to the merge button in the UI. Thus, to really protect committers from accidental merging, we should not allow all committers to bypass.

2. The opposite approach would be to only allow a minimum number of specific teams to bypass. To do this, we'd need to identify cases in which bypassing is actually required. I can see the following cases from most likely to least:

   1. When making CI changes, sometimes jobs fail before they are merged and only start passing once on master. There is no sensible way to avoid that in all cases, it depends on the kind of changes. This is on purpose.
   2. When making CI changes, sometimes jobs pass in the PR, but start failing on master. In this case, a follow up PR to fix them, might still have failing jobs. This is by accident.
   3. A job could be flawed and fail for a regular change, which is supposed to be fine.
   4. Jobs could start to fail randomly, for example because GitHub Actions is having issues.

Are there any other, possibly even regular, cases where PRs must be merged with failing checks?

For 2i and 2ii, the new NixOS/Nixpkgs-CI team needs the ability to bypass.

I'd argue that for 2iii, the job is not ready to be "required", yet/anymore. In this case, any regular committer can open a PR towards the CI workflows to propose removing the job from the list of required jobs.

When everything breaks down in 2iv: Do we actually want to keep merging stuff if we're not sure about the validity of the changes? I'd say no, if we care about CI results, then bigger scale GitHub issues just need to decrease velocity.

In NixOS/nixpkgs#416459 (comment), @MattSturgeon mentioned to assume that "other teams would also get bypass permission, e.g. the security team and org owners". Of course, org owners can always change the branch protection rules and disable them. According to the list above, so far, the security team would not need to be able to bypass.

Thus, assuming that only the CI team can bypass, we'll need to answer the following questions / concerns:

• In teams/ci: init nixpkgs#416459 (comment), @Mic92 mentioned that maybe the team could too small and thus the branch protection rules could lead to blocking others / we create an arbitrary bottleneck.
• In a hypothetical scenario, where a non-CI-team-member merges some CI-breaking changes, they can't go back and revert that.
• Others?

Regarding team size: Even though we only have 5 members, we also org owners on top, who can always disable the rules. I think that should give us enough people to have somebody available to react in the worst-case. Implementing a "only those who regularly need to merge non-passing PRs are bypassers" should be possible. This would only be the CI team right now.

Regarding the second case above: Maybe we could implement an exception for revert PRs. Those could bypass the required check. This would allow anyone to always recover from such an accident.

---

With all these things in mind, I propose the following steps:

1. Implement "reverts are always allowed".

2. At first, add all committers as bypassers for some time. Collect data, see how often the status checks are actually bypassed intentionally.

3. Later switch to only the CI team as bypasser. Adjust accordingly, if new requirements come up in step 2.

If we're confident enough, step 2 could also be optional.

[MattSturgeon]

• In the UI, the merge button is disabled (similar to when a PR is merge conflicted)

Note it is possible to still have a working merge button if you either enable "Allow Auto Merge" or enable "Require Merge Queues".

Until required checks have passed it will show as "Enable Auto Merge" or "Merge when ready".

This allows you to click the button early on a PR that looks good, without needing to bypass CI.

---

1. It's unfortunate that checks will always be bypassed from the command line - because merging from the command line is also in principle most likely to miss a failing check, since it's easier to see the list of checks right next to the merge button in the UI. Thus, to really protect committers from accidental merging, we should not allow all committers to bypass.

I agree that's unfortunate. I throws out some of my assumptions in NixOS/nixpkgs#416459 (comment):

Assuming it isn't easy to accidentally bypass, then I'm fine with either approach. If bypassing accidentally was easy, it'd kinda defeat the point of having a ruleset in the first place

Outdated rambling...

That said, this may be mitigated by requiring Merge Queue. IIUC that will block CLI merges; well you could use the gh pr merge command, but you know what I mean. (gh pr merge has an explicit --admin flag for bypassing protection).

While we won't be requiring Merge Queues straight away, it's worth considering if this changes any of our decision making now; or if it means we can initially have a strict policy that is relaxed once merge queues are in use?

EDIT: (To clarify, I'm not 100% clear on how Merge Queues interact with bypassing; does the queue also get bypassed or does it just allow adding a failing job to the queue? I assume the latter wouldn't make sense, because if failing, the queue itself would never be merged. I also haven't checked how merging with the git merge and git push CLI interacts with merge queues when you have bypass rights; so this entire section may be irrelevant 😅 )

EDIT2: I've thought about this some more, and I think my assumption "IIUC that will block CLI merges" is incorrect for bypassers; I suspect that bypassers can merge PRs without even waiting for the queue, if they either tick the red box in the UI, pass --admin to gh pr merge, or use git push to merge manually.

---

Maybe we could implement an exception for revert PRs. Those could bypass the required check. This would allow anyone to always recover from such an accident.

This sounds reasonable. Ideally it'd still be obvious that the "success" step was skipped because it is a revert and not because there are no CI failures. Maybe on such PRs we'd post an initial comment:

This pull request looks like it is reverting a commit.

To avoid needing to bypass protection rules, the PR > Success check will be "skipped". This does not mean that CI has passed. If there are failures, you should manually check each of them before merging.

Usually, you'd only want to merge with failing checks if this PR reverts the causes of those failures, meaning they will only pass again after this PR is merged.

---

With all these things in mind, I propose the following steps:

1. Implement "reverts are always allowed".

2. At first, add all committers as bypassers for some time. Collect data, see how often the status checks are actually bypassed intentionally.

3. Later switch to only the CI team as bypasser. Adjust accordingly, if new requirements come up in step 2.

If we're confident enough, step 2 could also be optional.

This plan seems reasonable, based on what you've outlined above.

[wolfgangwalther]

There is another case where required status checks need to be bypassed: When doing the periodic merges from master to staging-next, staging-next to staging etc.

None of those are done via PR - and thus they would be blocked, because the commit to be pushed doesn't have a successful workflow run associated, yet.

The periodic merges are often done by bot, but once we have merge conflicts they need to be manually resolved. So just having the bot be able to bypass won't be enough.

There's two ways to deal with that:

• Do those merges, when conflicted, via PR. That would very likely become quite annoying quickly.
• Keep (at least somebody) with the privilege to bypass for those merges.

I don't think we want to limit the ability to resolve those conflicts to a small group. Thus, I think, we need to always allow bypassing the required status checks for the staging-like branches. We could still enforce full protection on merges into master/release, eventually.

[MattSturgeon]

When doing the periodic merges from master to staging-next, staging-next to staging etc.

Note that you can have multiple rulesets, each with different bypassers.

So you could have a strict ruleset for master with a handful of bypassers and another ruleset for staging branches with additional bypassers.

Not saying we need this, but it is an option.

[wolfgangwalther]

Basic required status checks have been enabled. All committers can bypass right now. An explanation has been pinned in the nixpkgs repo at NixOS/nixpkgs#424273.

[MattSturgeon]

Please report any other cases where you needed to bypass the rules for merge in #130!

I've stumbled on a case where CI was blocked by nixpkgs-vet and needed a bypass: NixOS/nixpkgs#422077 (comment)

cc @drupol