Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 0156] No Direct Nixpkgs Pushes #156
[RFC 0156] No Direct Nixpkgs Pushes #156
Changes from 1 commit
25ce9d6
becc35e
1f44399
709c897
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these would also be addressed by "Require signed commits || Require a pull request before merging".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we use CI tied to PRs that depends on master evaluating with only specific deviations from cleanly, «be broken» does get easier to handle without direct pushes…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure signed commits doesn't make it harder to push malicious, broken or poor-quality code, why would it? The only thing it gets you is that commits can't be anonymous anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends, for who. An attacker needs to have access not just to your token or ssh key for pushing, but also the signing key. If I am correct it's also not possible anymore to make changes via the API as it's lacking the signing key. Of course, our contributors can always still push something broken themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah I didn't consider such an attack scenario here. I'll definitely add this appropriately into the Alternatives and Future Work sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can and does apply to any "forge" or any process for development.
It's not "must be github produced pull requests", but "tangible item that allows tracking the origin". If Nixpkgs had a mailing-lists development process, it would be "any change must be sent as a patch set / pull request", and thus provide the same tangible "item".
The principle behind the change does not entrenches into any specific tooling, it makes a pretty much already universal development process mandatory.
This leaves a common tangible "item" to discuss about the bad contribution. Thus, the overall already near-universal development process is defined.
@infinisil do tell if you think I've misunderstood something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're on point @samueldr. And while I do like the thought of switching off GitHub and signing commits, this is not the place to discuss it (I'll add it to Alternatives/Future Work though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also add to this that while GitHub has a UI for this, this is not GitHub specific. If it's later decided to host our own copy of the canonical Nix repository somewhere, you could still get the same feature this RFC asks for with a remote commit hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC does not say "tangible item that allows tracking the origin".
It says:
https://github.com/tweag/rfcs/blob/709c8979ece291291ff12da8e206cb212a14652e/rfcs/0156-no-direct-pushes.md?plain=1#L43
The text above specifically references git-hub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aw come on, this is needlessly pedantic quibbling. Yes this is a GitHub feature, and therefore the RFC text mentions (and should mention) GitHub. No this is not a GitHub specific feature, as most other git forges offer a 1:1 equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think this improves the situation from the point of view of separating we is the generic change and what is the GitHub implementation, @amjoseph-nixpkgs was rightfully complaining about the intermixing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a remark here that GitHub does allow merging PRs via a direct push of a clean-merge commit. (I cannot figure out from the documentation if it allows pushing an unclean merge, which would be an issue if we took «malicious code» part seriously)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will test this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if there are any merge conflicts, then the merge commit may contain arbitrary changes in order to fix that commit. Not sure if this is prevented on PRs with merge conflicts or if this could be prevented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, have you kept count how many obvious false positives you had to remove? To estimate how many non-obvious false positives we probably have remaining…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were 79 obvious false positive, which after another look are all from seemingly buggy merges using GitHub's web interface. Here's a complete listing:
Complete listing of false positives
Though I don't think this needs to be part of the RFC because it doesn't change any arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if you just add 79 to show that clear false positives are more numerours than «maybe, who knows», it does strengthen the argument about rarity of true directy pushes and their removal changing nothing at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I don't think it matters though, it's all just caused by imperfect ways to get at the right data (my script and GitHub bugs in this case). These false positives have no influence on any arguments and in fact only distract from the main point. In fact I should just "fix" my script to not count any commits from @web-flow at all, then I don't need to mention this at all because there aren't any false positives anymore.
Note that these false positives in my script are completely unrelated to the false positives in the automatic notifications in NixOS/nixpkgs#118661, which do have an impact because they annoy people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The false positives are relevant for two reasons.
ETA: also comparison of web-flow and non-web-flow «positives» now and a few years ago tells us that most of the 5% back then were true positives unless GitHub has dramatically changed the bug occurence ratio between cases without fixing the (better defined) webflow case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sternenseemann See this section. Pull requests don't need to pass CI before they can be merged. So if you would have committed directly before, you now just need to open a PR and merge it immediately, no need to wait for CI. (That's of course not a great workflow, but that's kind of the point of this RFC, making these things more discoverable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but my point still stands. If we are to force this workflow, it should also have benefits for the change author thelmselves—i.e. working CI. Everything else is just frustrating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current proposal only affects about 0.05% of commits, has effectively zero downsides, and can be implemented in 1 minute, making me fairly confident that we can get this accepted quickly.
While better CI would be nice, it would expand the scope to also affect the other 99.95% of commits, involve more tradeoffs and take much longer to design and implement. I'd rather split this into a separate RFC for the future. Let's take small improvements where we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you say, this change affects very little and doesn't change anything about the fact that we need to trust commiters. If they are fixing a typo in the manual, a PR gives them the benefit of confirmation that they did not accidentally wreck the manual build—all we are doing is forcing them to appreciate this benefit. This is not the case in the case of an eval failure on master.
If we are to make PRs mandatory in every case, we should also make PR CI useful in every case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we probably should, but this doesn't have to creep into the scope of this RFC. PR's are useful whether CI passes or not. If you have a concrete plan on how to improve CI, feel free to open a separate RFC, it's entirely orthogonal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, PR CI is useful for you, PR discoverability is useful for everyone else—hopefully our commiters are Kantians and not frustrated by this…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not entirely true. Yes, the manual merge into staging-next is done with PR. However, there is also a workflow that automatically merges between master, staging and staging-next. And, as merge conflicts are a thing, these are often resolved with direct pushes.
Regarding the workflow for automatic merges. Can that still be kept? I am thinking here about a potential next step, commit signing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is conflict resolution using direct pushes to staging/staging-next, or also to master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I see now that
staging*
is excluded. Generally speakingstaging-next
tomaster
can be resolved by mergingmaster
intostaging-next
first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the initial merge from staging into staging-next is done manually. We only use a PR from staging-next into master.
Yes, merge conflict resolution is usually done with direct pushes to staging-next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I meant that one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or any other wording to help resolve the ambiguity at first read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another important point is that in the years since #79 the discussions lead to various workflows to migrate gradually away from direct pushes. The old RFC proposed a step change unaligned with then-current practice (also it wanted reviews but whatever), the new RFC proposes to finalise a process transition that has had time to be almost completely implemented in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes a bit of time, but I can also check how many commits were directly pushed to master back then, I wouldn't be surprised if it was already very low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was pretty low for sure, but noticeably higher — and the updates to
staging
workflows in the meantime probably matter.Ah, maybe I can put it like that: back then, there were some true positives in the direct push tracking action — comparable amounts to false positives, nowadays this doesn't seem to happen at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@7c6f434c are you saying that customs changed between #79 and now? (If so I would be curious why). Or was there something else that makes the change more palatable now than it was back then? (other than the review requirement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back then, the majority of changes already went through a PR at some point on their way to the main branch. For the exceptions, there were a few years of advertising specific changes to the specific workflows while talking up the virtues of ofBorg eval check until everyone either agrees or is nagged into compliance; apparently after all the workflow work the strong arguments for direct pushes to the channel-feeding branches are resolved.
This is the difference between «now» and «the current RFC transplanted back in time with the data re-processed according to the moment of submission». «RFC 79, try again» would hopefully fail again, because of the other demands it also had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the results, out of a total 50457 master commits in the last year at RFC 79 start time 1, at most 2682 of them (5.315%) were direct pushes to master, complete listing here (not counting the 153 obvious false positives from @web-flow, see #156 (comment)).
So while it was still about 10 times less than the original estimate in the RFC (46.85%), it was 100 times more than now (0.0517%)! I will update the RFC with this information.
Footnotes
Unix epoch 1569369600 to 1600992000 ↩