[RFC 0079] No more direct pushes to master and release branches #79
Conversation
|
|
||
| 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. | ||
|
|
||
| By changing our branching workflow to a no-push-to-master workflow, we can achieve more security, stability and even more important: better scalability. |
There was a problem hiding this comment.
How does this improve scalability (of what?) when this obviously introduces more steps?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Quickly noting (before reading) that this has been discussed lightly in previous instances on IRC and it would cause considerable friction in the workflow of contributors, including contributors that use the GitHub platform. One issue (IIRC) was with signing merge commits. Another was from automated updates like @NeQuissimus does for Linux. It would require significant changes in the workflow, and probably cause updates to not be merged as promptly as they are now. All that is also not considering those users that (rightly so) want to engage with GitHub as little as possible. (Now to reading!) EDIT: With that said, it is important this RFC still goes through the process; these are simply reported sentiments and problems from previous less formal discussions. |
| - nixos-* | ||
| - nixpkgs-* | ||
| - release-* | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
rfcs/0079-enforce-prs.md
Outdated
| # Detailed design | ||
| [design]: #detailed-design | ||
|
|
||
| In GitHubs [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. |
There was a problem hiding this comment.
Do required reviews include those of us that are only maintainers and not committers? If not, I don't think requiring reviews is a good idea -- there is already a shortage of people merging Nixpkgs PRs; requiring them to approve before merging means they should spend the time testing it themselves and cannot just rely on e.g. the package maintainer's approval.
There was a problem hiding this comment.
I think it is a good idea to protect the branches against direct pushes. There is however a use case which is important to me. I often merge the staging(-next) branches into master and vice-versa. Depending on how involved I am this happens daily. Pushing this to GitHub, and then waiting for CI just takes too much time. Thus, unless we let a bot do the bulk of these merges, this should remain possible manually otherwise it simply won't happen.
Requiring approval of Pull Requests is orthogonal to the title of this RFC and should in my opinion be considered in a separate RFC, not here.
|
|
||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. I don't see any conflicting points between your statement and mine. 😄
There was a problem hiding this comment.
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.
| (`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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, what does GitHub squash do?
There was a problem hiding this comment.
I checked a repo where people squash and GitHub's squash rebases the result of the squash
There was a problem hiding this comment.
This is IMHO the worst anti-pattern github introduced in the recent years, as this effectively destroy history!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
|
||
| 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. | ||
|
|
||
| By changing our branching workflow to a no-push-to-master workflow, we can achieve more security, stability and even more important: better scalability. |
There was a problem hiding this comment.
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.
I see your point and agree. |
|
I wish GitHub had more fine-tuned permissions for this, because I think direct-pushes to release branches should be allowed -- but only by release managers. |
|
Personally, I do not care about stable releases but I cherry-pick fixes to stable branches when reasonably certain it will not cause issues and someone asks me for it. If this RFC required me to open a PR, wait for CI to pass, and only then merge it, I would never backport anything. The same would happen for other small things like typo fixes, I just could not be bothered. Or the master→staging-next→staging merges Frederik mentions. From the previous discussion of this policy, I am not the only one who considers this too much overhead to be worth it for small changes. Really, we need an automerge bot before this can be considered. |
|
@jtojnar I completely agree with you. I entirely forgot about the backporting case. |
|
@jtojnar I knew I was missing one important part of those previous discussions in my initial comment. And you're right, for backports multiple contributors said outright they just wouldn't bother. |
|
I pretty much agree with what @samueldr said above. I unfortunately think that no kind of GitHub provided configurable policy is really helping us here. It will only come with downsides that we can't (easily) mitigate otherwise. Our primary lack of resource is human attention and by requiring more of that we might end up with less people willing to put up with the work. While I agree that every commit should go through CI and a PR I can also see cases where that, and the review requirement, can be counter productive. That being said I think we should focus on the branch protection in this PR and not the review requirements. Both of these should have their own discussion. |
|
A few things off the top of my head: Have we considered something like Mergify or a GitHub action (or teach @ofborg about this)? I would imagine the workflow similar to this:
The real issue (apart from the special cases with I'll make it specific in my case: The kernel updates Maybe a test-and-automatic-merge workflow would also lead to more/better tests, something we can definitely use :) Edit: Some of these things have already been mentioned, I just wanted to ensure my train of thought made sense |
|
@FRidh wrote—
@jtojnar wrote—
When I saw the title of this RFC, I was expecting it to propose the use of such a bot as Rust's Bors (which remains the state of the art as far as I'm aware), but, as far as I see, the nearest it comes to suggesting such a thing is a single, unelaborated mention of "tooling" in a non-normative section, namely—
I was disappointed to see that this RFC does not discuss "prior art", i.e., at least one other large repository that (successfully) operates with this requirement, such as Rust. I suggest that this RFC would do better to support itself with a discussion of "prior art" and to propose, normatively, the use of tooling such as an "automerge bot". |
|
Further discussion/input about merge bots can be found here:
/cc @kevincox probably should be involved (as co-author) either in this RFC or in a sister RFC |
There was a problem hiding this comment.
Additionally, most of the previous discussions of that end up with the outcome «we need a working merge bot first».
Overall I find that this proposal is not yet in a shape where it brings anything new to the discussions that have previously happenned a lot of times, and unless significantly reworked is likely to end up in the same state — stalled because realistically likely improvements do not gain a reliable consensus of being large enough improvements to be worth further throughput problems. I am fine with this outcome, and hopefully the discussion will increase our chances of ever getting some merge automation (which, of course, will open some new and interesting security issues, but will still improve the overall safety on the balance due to faster processing of backlog of fixes).
| (`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. |
There was a problem hiding this comment.
Also, what does GitHub squash do?
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| It might break the workflow of some committers which are only a small portion of the community. |
There was a problem hiding this comment.
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.
|
|
||
| It might break the workflow of some committers which are only a small portion of the community. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
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.
| (`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. |
There was a problem hiding this comment.
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.
| # 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| 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. | ||
|
|
||
| By changing our branching workflow to a no-push-to-master workflow, we can achieve more security, stability and even more important: better scalability. |
There was a problem hiding this comment.
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.
|
The problem I see is that To join the discussion I'd like to share a few of my thoughts I had in the past about it[1]:
Instead I'd suggest something like this (note that these are just some ideas, not a detailed plan :)):
[1] Disclaimer: I'm sharing my personal experience as relatively active maintainer & committer, I don't intend to speak for all other developers. Also, it's possible that I forgot about something here :D |
|
It is very clear to me that the policy proposed in this RFC, after an initial settling period, would shift the needs of core maintainers towards improving PR workflows. This would put core maintainers into the same boat as "the rest of us" with regards to PR workflows and align interests, instead of preserving special priviliges/workflows which ultimately result in negative overall outcomes. Oposing some of the oposing views expressed: hen or egg doesn't really (=lastingly) matter. I strongly think, this RFC should move forward and provide transitionary provisions just as any good legislation does. |
Under the must-get-reviews scenario there are two cases: the amount of human reading increases (or, more realistically, more PRs are let to fall through cracks), with no way to change this by workflows; or people start approving without reading code. Do you claim that this does not happen? It is obvious to me that the less-frequent contributors will suffer first in the former case. Committers can trade reviews, after all… (Note that true security benefits in case of compromised-committer scenario will never materialise because requiring two reviews on non-maintainer contributions will get cancelled in around 72 hours as unsustainable, and sockpuppets are cheap) |
|
Thanks for looping me in. I'm strongly in support of this idea but it is clear to me that there is a lot of infra work to do first. I've been (slowly) working on preparing a set of RFCs that will move us there. But it will take a long time, and non-trivial amounts of work. My progress so far is here, but I'll summarize the major points I think we need.
Furthermore if we want always green we need a policy for flaky packages and a way of handling arch-sensitive packages. I'd love to continue to make progress here. I'm hoping to submit the breaking change policy as an RFC soon. I think it lays the groundwork for the rest. Although we will need to polish up the tooling before it can really take effect. |
I'd add: over time, the ecosystem would adapt towards better practices (principal motivation of this RFC) and find it's new optimum (most likely heavily backed by tooling and improved workflows). I claim: we are currently stuck at several local optimums all over the place. As a result, adoption of nix and its philosophy in the wild is not as dynamic as it could be.
This is a problem, a code of conduct can counteract. |
|
@blaggacao Of course, I'm in UTC+2, so whatever works for you (and is not at 4 in the morning for me). @matthiasbeyer is probably also interested |
|
I'd add: over time, the ecosystem would adapt towards better practices (principal motivation of this RFC) and find it's new optimum (most likely heavily backed by tooling and improved workflows)
Or, with a freshly organised PR disaster in place, handing out commit rights can finally reach the pace it should.
I mean, if you have a specific proposal to use an available GitHub Action for a mergebot with specific configuration, I will be glad to help you polish it, and I believe this should be possible to push through. What you propose, though, is breaking things to make a point.
> Committers can trade reviews, after all…
This is a problem, a code of conduct can counteract.
Counteract what? Agreeing to help each other on an exchange-of-effort basis? The reviews will be honest, that is not a problem.
|
|
I agree we shouldn't change too much too fast, but I also would much rather spend our "change budget" on things coming out of this like that could save serious core contributor time (e.g. auto-merge bot), rather than things like flakifying Nixpkgs which perhaps could increase adoption and look flashy but I don't believe address our core issues around the sustainability of the project. (To be clear when I say "core contributor" I'm talking about the people that deal with the daily drudgery and that does not include myself.) |
|
I have high hopes that we can first get some of the tooling where change budget cost is low (implementation cost is another story, though) |
|
Change Budget is a good mnemonic term for use in future policy discussions. Note that it can not only be leveraged by tooling but also expanded by a strong shared vision. For the record: my point about flakes was that it might be an endogenous as well as exogenous dynamic to unfold that the (core) ecosystem can't but react upon. That might put even more backpressure on existing workflows, so we are well adviced to plan ahead. |
|
There seems to be a consensus about auto-merge bot. I invite anyone interested in investing time and energy in making this happen to PM me on discourse with the goal to discuss (sheltered from public scrutiny) the constituing individual's personal feasibility to participate in the conformation of a special interest working group. A special interest working group seems like a good enough answer to productively channel the dynamic this discussion has developped. |
|
Looks as if mergify is willing to help the community. @NeQuissimus would you be interested in helping to set up an example configuration to have a basis for discussion and see what's possible? For conducting an experiment, I think it might help if it is narrowly scoped (and does not produce external effects on parties not yet accustomed with this workflow), so I would suggest to come up with a configuration for the subtree containing the things you maintain. What do you think? |
I think that GitHub is an unfixable mess as they break their own API promises without updating documentation even in a reasonable time after the fact. Hence an external proprietary merge-bot with good reputation is probably not making things any worse than just the (bad for a ton of reasons, but expensive to change for another ton of reasons) fact of using GitHub, in my opinion. I am afraid that giving write access (which, I think, GitHub cannot limit to subtree) to the repository to an externally developed bot would require quite detailed write-up of what we know about all the moving parts. The knowledge might be imperfect but people will ask for what we do have… Do I understand correctly that we are supposed to give the bot a moderately long-lived token granting push access? (Or does Mergify need a procedure that implies other permissions) Does GitHub support limiting the token to subtrees? Can the token be GitHub-side limited just to How token rotation works? Do I understand correctly that a Mergify has long-supported option to never push anything touching anything outside the configured subtree? How subtree reconfiguration works? How many people could be managing the Mergify configuration? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/steam-engine-bors-for-merge-train/9676/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/meeting-minutes-2020-10-23-future-of-and-by-rfc79/9680/1 |
|
As a result of this RFC's discussion, part of the discussion evolves under the newly conformed SIG Workflow automation and parts of the reverberations of this RFC's discussion are picked up in:
Please feel free delivering your own "Geistesblitz" or "Steam Engine" through the templates provided in the footer of the SIG Workflow automation description page. Keeping the discussion going is an important intrinsic motivator for all of us. Thank you all for your contributions! ❤️ Please let's agree to leave any detailed actual policy decision to any sort of future SIG Trust model — I encourage anyone interested to propose its conformation, though since its input is relevant for any kind of workflow automation attempts that want to go further than the status quo. |
|
NOTE: If you are a committer and want to avoid accidents, it's possible to disable the push on the client-side: git remote set-url --push origin no_pushOr just for the master branch: git config branch.master.pushRemote no_push(posting this as a workaround until we can enforce it server-side) |
|
I'd like to propose a different approach by taking smaller steps. A good example of what motivated me is NixOS/nixpkgs#112477 where the PR is stalled because there are no active maintainers of that stack. The proposal is to use an existing bot to merge PRs approved by maintainers specified in CODEOWNERS. This is simple to implement (a few lines of YAML + a token), most of the work is documenting the policy (RFC). The benefits would be that we're slowly moving towards understanding better who is the maintainer of what part of nixpkgs and that we don't need to give out full commit access to those that want to merge things. On top of that it's all transparent. I'd like to hear your thoughts about this as a reasonable way forward. |
|
Does that necessiate a brief review of CODEOWNERS now that it will actually do something? In favour either way. |
It would mean looking at #50 again and making the case for the suggested simpler method. |
|
Strongly in favour of introducing an owners concept and automatic (or self) merges. The benefit of the latter should also not be understated, it's a small psychological one that is known in large orgs. Imagine the case of a PR that touches a range of different things and needs review by multiple people, but the "least significant reviewer" does their review last: In a model where completed reviews just "grant merge permission", the owner of the change (or a bot) can merge as soon as all reviews are done. In our current model, the owner of the change now needs to poke one of the other reviewers again (or find someone else willing to do the merge). Not granting treewide commit access to every contributor is also great! |
And, because treewide commit access happened before, reducing the number of people who actually have treewide access in the same step! I love the idea of using CODEOWNERS - I just hope that the bot @domenkozar mentioned works well enough, so that if multiple files from different owners are touched, all owners must review (or at least that part is configurable with the bot and we can see what works best for us). |
Skimming through it it's just a few people that don't already have commit access.
Good point, all that feedback should be taken into the account.
It doesn't do that, although I wouldn't complicate it much, if we survived with the merge button we can start with a The big question then comes, who can become a maintainer? That's the actual big part of the policy that needs to be written. |
As a first rule of thumb: If I add a new package, I should automatically be maintainer of that package (and by that I do mean It might also be a good idea to copy |
|
Right now,
It'd definitely be possible, but it'd mean that any codeowner of any part of the code technically has the ability to land code literally anywhere in nixpkgs; so the security benefits of not giving everyone treewide access would be very limited… and it might actually become a security issue, because we'll start assuming that codeowners can only touch the stuff they are owners of and that would not be true. Because we'd start giving code ownership much more liberally than we currently do for the commit bit. IMO the core of the idea is good, but we probably should 1. make sure that the bot requires review from at least one owner of each touch filed (otherwise it'd become a security liability, see paragraph above) and 2. start with code owners that are (a subset of) committers, and stay very deliberate in how we grant code ownership. |
How about having a rule where if there is a single maintainer to the package, then one approval is enough to merge. If there are 2 or more, then you need at least 2 approvals. You can definitely still game the system by creating many accounts. I feel thought that it would take care of 90% of the cases where there would be a problem. Another solution to the problem, would be to create an
Elaborating on the previous idea, you could make codeOwners required to review every PR. The more strict requirements would leave the more committed. The problem would arise when a codeOwner steps down and there is no one to replace him/her. |
opened, synchronize, reopened are the defaults for `pull_request_target`, `edited` will trigger the label action if the PRs base branch is changed.
|
Does anyone care to provide a summary of:
My impression is that an RFC that gets us one step closer to a Good PlaceTM got stuck in a not so fruitful, very complex discussion, and that we let perfect be the enemy of good, as they say. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/improving-the-commit-access-system-for-nixpkgs/16135/1 |
|
New RFC for this but slimmed down and updated: #156 |
Rendered
cc @matthiasbeyer @lschuermann