Skip to content

Latest commit

 

History

History
184 lines (146 loc) · 17.7 KB

0156-no-direct-pushes.md

File metadata and controls

184 lines (146 loc) · 17.7 KB
Raw
feature start-date author co-authors shepherd-team shepherd-leader related-issues
no-direct-pushes
2023-07-21
Silvan Mosberger
(find a buddy later to help out with the RFC)
(names, to be nominated and accepted by RFC steering committee)
(name to be appointed by RFC steering committee)
(will contain links to implementation PRs)

Summary

Require pull requests for all Nixpkgs commits.

Motivation

There are currently 197 people with commit access to Nixpkgs, all of whom can push directly to any branch without a pull request. Such pushes generally1 do not notify anybody and do not trigger CI.

This makes such commits susceptible to:

  • Be anonymous2
  • Include malicious code
  • Be broken
  • Have poor code quality

While requiring pull requests isn't a panacea, it does help by:

  • Running CI (though there's no requirement for it to succeed, see future work)
  • Requesting reviews from Nixpkgs maintainers via CI
  • Requesting reviews from code owners
  • Tying commits to a GitHub account
  • Being discoverable in the pull request list using various filterable and sortable metadata such as (manually and automatically assigned) labels, update time, authors, reviews, etc.

Detailed design

Turn on GitHub's "Require a pull request before merging" branch protection rule for all branches whose commits get propagated into channels. This includes:

  • master: Used for unstable channels and branched into new release branches
  • release-*: Used for stable channels

Staging branches are intentionally not included, because they will already require a pull request when they inevitably need to get merged into one of the above branches. The same applies to similar long-term branches like haskell-packages.

A NixOS GitHub organization owner needs to implement this change and should therefore review this proposal.

Disable the direct push detection workflow

There is a GitHub Actions workflow to detect directly pushed commits. When detected, it creates a comment in the commit pointing out that this is discouraged and linking to this issue, where it's easy to see all direct pushes. The script occasionally has false positives3, which creates some unnecessary commit comment notifications.

With this proposal accepted it won't be possible to directly push commits anymore, making that workflow unnecessary. It can be removed and the above two issues can be closed.

Examples and Interactions

Direct pushes listing

Out of the 112217 commits to master in the last year4, at most 58 (0.0517%) of them were direct pushes.

To determine whether a commit was pushed directly, the GitHub API was queried for pull requests associated with that commit (see associatedPullRequests). If this list includes a merged pull request to the Nixpkgs master branch, the commit is known to be merged with a pull request. Otherwise the commit could be directly pushed or be a false positives, which is why the above count is only an upper bound. All obvious false positives (example) have been removed from this count and listing already.

Complete listing of the 58 potentially directly pushed commits in the last year

Note This was generated with a fairly hacky and non-reusable script, but it can relatively easily be verified probabilistically by picking a random commit in the time range and checking if it belongs to a pull request, repeat to increase confidence.

Emergency changes

Sometimes channels have blocking breakages and need to be fixed as soon as possible (citation needed). Currently this can be done with direct pushes, but a pull request will be required with this proposal. The time required to fix such breakages however is barely affected: Since there is currently no requirement for pull requests to be approved or pass CI, they can get merged immediately after opening if necessary.

Staging workflow

The staging workflow is not affected because it already uses pull requests for all merges into the affected branches.

Drawbacks

  • Even trivial changes that don't need a review now require a pull request, which takes more effort and creates noise for reviewers.
  • It takes slightly longer to push a quick fix in emergency situations.

Alternatives

  • It would be possible to implement a third-party interface to de-anonymize future commits (even if pushed directly to master) using the push event GitHub webhook, which includes the sender field to match the pushing GitHub user.
    • This would not solve the other problems with direct pushes though: It still wouldn't notify others, trigger CI or be discoverable.

Prior art

The previous RFC 79 attempted to do the same, but:

  • It had a mistaken estimate for the percentage of direct master commits, calculating it to be 46.85% in the last year. The mistake was assuming that all non-merge commits were direct pushes. This made it seem like the change was much more impactful than it actually would've been.
  • It was too ambitious by also proposing to require accepting reviews for all pull requests.

Unresolved questions

Future work

More restrictions could be implemented in the future:

  • Require all pull requests to have at least one approval, therefore preventing people from merging their own pull requests (it's not possible to approve your own pull request)
  • Require an approval by a code owner, properly establishing code ownership over all parts of Nixpkgs
  • Require CI to pass, maybe using GitHub's new merge queue's

Footnotes

  1. There's no GitHub mechanism that takes care of notifying other people and there's no builtin way to get notified for new commits linked to the direct push issue, but third-party tooling could be implemented to get notified explicitly for direct pushes ↩

  2. GitHub does not require committers to match the pushing GitHub account, here's an example. ↩

  3. https://github.com/NixOS/nixpkgs/issues/240314 ↩

  4. Unix epoch 1658361600 to 1689897600 ↩