Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0079] No more direct pushes to master and release branches #79

Closed
wants to merge 2 commits into from
Closed

[RFC 0079] No more direct pushes to master and release branches #79

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21e0fde
[RFC 0079] Initial work from our CryptPad session
dasJ Sep 25, 2020
cdd088e
Add review improvements
dasJ Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions rfcs/0079-enforce-prs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
---
feature: No more direct pushes to master and release branches
start-date: 2020-09-25
author: Janne Heß <janne@hess.ooo>
co-authors: Matthias Beyer <mail@beyermatthias.de>
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: None
---

# Summary
[summary]: #summary

Enforce usage of Pull Requests for **all** contributions to nixpkgs master and release branches, which implies that these branches do not allow direct pushes anymore.

This implements the four-eyes principle and allows easier change discussion, both before and after the merge, and improves overall security since each Pull Request has to be approved by at least one other person prior to merging.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request has to be approved by at least one other person prior to merging.

This was discussed on the merge bot RFC as well. The group of people with push rights is too small. There we proposed introducing a new group, trusted reviewers, that could approve and then merge via the bot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think requiring 4-eye-review for stable branches will in fact improve the situation there at all. From subjective observation there appear to be only at most a hand full of people caring for a released version of NixOS besides some random back ports and high visibility issues. During the 19.09 & 20.03 release cycles there were multiple cases were PRs to fix security related stuff had been open for days. If I wouldn't be able to merge my own PRs (even without CI as they might be rebuilding everything due to touching OpenSSL) I'd probably not have done that work (and they might still be open until today).

We can probably discuss how useful protecting the branch against (fast-forward) pushes is but the review requirement should probably get a dedicated RFC (and discussion).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group of people with push rights is too small.

No, it is too big. That's the whole point of this RFC - too many people can break master/release and push security issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not have enough people with merge access to merge all of the slightly lower profile security fixes in a timely fashion. (Also, we lack enough people to triage security and general severity of bugfix releases, so we need to somehow speed up merging almost all of the minor updates).

Tricking people into merging non-obvious security issues will only become easier by increasing the amount of rote process.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't see any conflicting points between your statement and mine. 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do not have enough throughput, putting speedbumps to workflows of people providing the throughput is a bad idea. This proposal is doing that.


# Motivation
[motivation]: #motivation

In its current state every [Nixpkgs committer](https://github.com/orgs/NixOS/teams/nixpkgs-committers/members) (139 people as of writing) is able to push arbitrary code to every branch, including master and all release branches.

Pushing to critical branches of a software development project is not a good practice in more-than-one-contributor environments.
Security vulnerabilities, breaking changes which are not tested, and regressions are easily introduced this way.
Those issues can been prevented by a proper workflow and tooling that comes with the workflow.

In the last year (as of 2020-09-25), we had
- 19471 commits on the master branch in nixpkgs
- of which 10347 (53.14%) were merges
- of which 9124 (46.85%) were non-merge commits

<small>
(`git log --oneline --since="1 year ago" --first-parent [--[no-]merges] | wc -l`)
</small>

This is an improvement over the previous statistic from 2019-04-13 (where 51.85% commits went directly to master) and proves the point that committers are able to work properly without pushing directly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I typically don't merge PR's but rebase the commits onto the target branch to avoid the noise of merge commits. I don't think these metrics can be used to distinguish between rebase through GitHub versus direct push.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what does GitHub squash do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked a repo where people squash and GitHub's squash rebases the result of the squash

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is IMHO the worst anti-pattern github introduced in the recent years, as this effectively destroy history!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not want the history of nixpkgs to have 50% merge commits to be honest. magit can barely display it as-is, no reason to clutter it with even more merge commits. If anything, we should encourage committers to use a rebase merge for 1-commit or 2-commit PRs that don’t introduce a critical change.

The PRs can still be found by searching Github for the commit id, and the default merge commit Github produces are rather simplistic (and don’t contain much information besides the PR number, not even a link to the PR).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, we should encourage committers to use a rebase merge for 1-commit or 2-commit PRs that don’t introduce a critical change.

I'm not a fan of rebase merges, tbh. I'd rather propose a daily or weekly collecting-small-stuff-PRs. But that's another RFC and we shouldn't discuss this here, IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this growth looks more like an increase of long-tail contributions and improved efforts on merging these PRs, not like a change in workflows in the widely important areas.


By changing our branching workflow to a no-push-to-master workflow, we can achieve more security, stability and even more important: better scalability.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this improve scalability (of what?) when this obviously introduces more steps?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gives us the ability to have an evergreen master, which in turn makes the whole "Well, the broken CI is unrelated and I think we can merge!"-thing go away, because CI suddenly means something. Green means good, red means a maintainer does not even have to care!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The times that something is broken due to a push is very small. Not to say we should not avoid it, but with the level of direct pushes we have nowadays it hardly every happens and tends to be corrected soon. Thus, at this point I disagree with it improving scalability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the proposal as it stands will not achieve any usefully always-green master. We do not have any pre-merge CI that would always catch either a Chromium build failure or all of the possible rev-dep builds failures, and are not likely to have such in the forseeable future.


# Detailed design
[design]: #detailed-design

In GitHub's [branch protection](https://github.com/NixOS/nixpkgs/settings/branch_protection_rules) rules, branch protection rules which require pull request reviews, include administrators, forbid force pushes and branch deletions must be created.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write an actual detailed design of the proposed rules.

Right now different parts of the proposal leave incompatible impressions on what is actually proposed. Just no direct pushes? Review requriements? How much of them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Also we need descriptions of every way the direct-push to master/branches is used by contributors at the moment, and what workflow to replace them with if applicable.

There must be rules for:
- master
- release-*

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should at least be some mention of what happens with other critical branches like "staging" and "staging-next". I don't know how these branches are updated currently, but I'd expect that workflows surrounding them will have to change.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. As far as I know, CI only ensures the staging branch builds before merging it, but not whether the actual merge builds, does it?
Because that would be (my) major concern here. Other than that, from what I know, I don't think the workflow should be changed. My concern with this PR is to get an evergreen master (and release branches), which (IMO) increases the development speed and scalability. Staging does not fall under that concern.

# Examples and Interactions
[examples-and-interactions]: #examples-and-interactions

This only affects nixpkgs committers.
When pushing to a protected branch directly, they get the same message as everyone else and they have to push to an unprotected branch and create a Pull Request from there.

# Drawbacks
[drawbacks]: #drawbacks

It might break the workflow of some committers which are only a small portion of the community.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question should be whether they do the majority of work for some specific areas, though.

Surely it is easy to me to go through PRs now that my activity has dropped and I rarely do more than one thing at once (which is often exotic enough to self-merge). People who try to quickly do a large amount of critical work will be more hindered than me.


Also, Pull Requests might take a bit of time before they are approved by somebody else, which shouldn't matter too much since the trust in committers is already very high and their Pull Requests are likely to be merged fast.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically, we have the norm that the committer PRs to leaf packages are unlikely to be reviewed ever unless someone has a very specific workflow to test.


# Alternatives
[alternatives]: #alternatives

Do nothing.
This has the downsides mentioned in the motivation and weakens the perceived trust in the project.

# Unresolved questions
[unresolved]: #unresolved-questions

Whose workflows will break?
Why are they not adoptable to a Pull Request workflow?

# Future work
[future]: #future-work

Handle complaints from people who are used to direct pushing.

Maybe increase the number of required approvals in the future. This however requires more active committers or Pull Requests are stalled for long times.