This comment made me wonder if the branch protection policies have been documented.
Looking at the list of branches, some of them appear ot be protected, but AFAICT it’s not possible to see what the protection amounts to.
Specifically: is it possible to push to master? And if it is, should it be?
I think the argument of convenience should be evaluated quite seriously, considering the implications it has for security.
Has this been discussed already?
Specifically: is it possible to push to master? And if it is, should it be?
Yes it currently is.
Forcing PRs with ofborg eval checks would probably not break too much, if ofborg outages are rare enough.
Forbidding self-merge will of course make the already non-perfect situation with reviewing non-committer package-addition PRs worse (for conservation of energy reasons), but this is a different step.
I think the argument of convenience should be evaluated quite seriously, considering the implications it has for security.
Has this been discussed already?
I think that it was discussed multiple times, but maybe too long ago
Yes, its possible to push to master and yes, this has been discussed plenty of times (at least on IRC, here’s an old related example) already. Its mostly a relic of the olden times when the project was much smaller. Some contributors started then, and old habits can be hard to break. The norm has shifted though, in great part due to @grahamc’s work (ofBorg adds a big incentive to PRs).
Maybe it is time for another discussion. I’d be in favor of branch protection. Contrary to the linked comment, I would not require an approval of a second reviewer though. At least not at first. That’s much more invasive and worsens our backlog problem. It may still be worth thinking about, but one step after the other.
Non-fast-forward pushes aren’t allowed (and deletion probably?) Maybe that’s the only current protection; I certainly can’t remember any other practically encountered effect.
Merging staging(-next) branches would get really annoying if that always had to be done through the UI. Other than that I am all for blocking direct pushes to Nixpkgs.
Don’t you typically create a PR for those anyway? EDIT: to be clear, I’m still not convinced we should use a hard rule here to disallow, but I haven’t thought about this in detail recently.
Only for the final staging-next to master merge, not for all the merges in between.
I didn’t realize you referred to protection of staging-next (or even that the branch’s protected already).
So you’re advocating for not protecting the staging-next branch, correct?
Right, I mixed them up.
Yes, or at least have a small group of people that can push to staging(-next) without going through a PR just for this purpose.
My workflows for backports happen to almost always be direct pushes on the release branches. I’d want an exception for that, even though you can still benefit from the evaluation checking. (but for anything greater than a simple update, I usually use a PR)
I’d say that protecting master is a straightforward, simple win with relatively little downside. For things like release-20.03, I think it’s probably more OK to have maintainers cherry-pick and push directly. Same for the merges between the various staging branches. At least for now.
Yes, let’s go with the least-controversial change now. We can think about going further later. Once we have more automated tooling (for backports: one bot that automatically creates a PR with the cherry-pick, another that automatically merges once CI passes) the cost-benefit there may become more obvious too.
I am opposed to preventing direct pushes to master.
There are all sorts of small things best fixed with a direct push. If I
had to open a new PR for stuff like
https://github.com/NixOS/nixpkgs/pull/82586 I probably wouldn’t bother.
Keeping track of open pull requests, knowing which ones need review,
etc. is difficult enough as it is without polluting the list with all
the stuff that is currently pushed directly.
I think it would instead be better to monitor what was being pushed
directly to master. It’s possible to identify which commits were not
part of a PR through the GitHub API.
I respectfully disagree.
It seems to me a commit like the one you posted is squarely a case of something that should go through a PR. I could imagine a similar commit straight to master introducing a security vulnerability by mistake, with the only safeguard against that being one person’s attention, and I think that’s hardly enough.
OK, but what should happen then? Someone goes and reviews all already-pushed commits to master looking for problems? Then it seems better to have the check beforehand, at least it’s mandatory, and we avoid having problematic commits in master in the first place.
I understand the frustration with having to wait potentially weeks (!) for a PR to get merged, but this is a general problem we all have - it seems slightly unfair for committers to have preferential access in this case. I can see how exceptions might be necessary (e.g. for security hotfixes), but version bumps like the one linked above IMO don’t fit the bill.
The backlog is clearly too big, but I don’t think this is the right place to fix that.
asymmetric via NixOS Discourse nixos1@discoursemail.com writes:
I respectfully disagree.
It seems to me a commit like the one you posted is squarely a case of
something that should go through a PR. I could imagine a similar
commit straight to master introducing a security vulnerability by
mistake, with the only safeguard against that being one person’s
attention, and I think that’s hardly enough.
I think you might misunderstand my point.
I was not the author of that PR, but the merger. I had to squash it on
the command line, because it needed more than just a squash into a
single commit, which is all GitHub supports.
If the submitter had checked the appropriate checkbox, I could have
pushed the changes to the PR and then merged, but I don’t see that
that’s any difference to committing directly. Am I supposed to open
another PR that’s just a squashed version of the other one, and then
potentially wait for yet another person to come along and review that?
OK, but what should happen then? Someone goes and reviews all
already-pushed commits to master looking for problems? Then it seems
better to have the check beforehand, at least it’s mandatory, and we
avoid having problematic commits in master in the first place.I understand the frustration with having to wait potentially weeks (!)
for a PR to get merged, but this is a general problem we all have - it
seems slightly unfair for committers to have preferential access in
this case. I can see how exceptions might be necessary (e.g. for
security hotfixes), but version bumps like the one linked above IMO
don’t fit the bill.
Committers already have preferential access. That’s what being a
committer is. I can merge my own pull requests, and if you’re proposing
changing that, then I think you’re going to have a lot of very unhappy
committers.
In the particular case you linked to, I think it is best to teach the contributor to make the change themselves. You already did that admirably well 
You could have just skipped committing the change yourself. The contributor could have easily repeated the process you showed them, improving the learning experience.
An alternative would be to commit the change to your own fork and give the contributor instructions:
git remote add alyssais git@github.com/alyssais/nixpkgs
git fetch alyssais
git reset --hard <your-commit-sha>
But I think the first version is preferable. The contributor learns something, gets to practice it and keeps control of their PR. Ignoring the branch protection for a moment, I always have a slight unease about editing someone else’s work and committing it without their feedback. That’s subjective though.
But I think the first version is preferable. The contributor learns
something, gets to practice it and keeps control of their PR. Ignoring
the branch protection for a moment, I always have a slight unease
about editing someone else’s work and committing it without their
feedback. That’s subjective though.
The contributor was about to go away and abandon the PR, as far as I
could tell. I would absolutely have done that in most cases, but here
it seemed pretty apparent to me that if I didn’t just merge it we’d just
have yet another open PR sitting there doing nothing.
I actually do the same thing, forgot about that *headscratch.
I haven’t considered that would actually make a certain portion of merges I do more difficult when I can’t push to people’s PR’d branches. Though we could add a message in the PR template to allow maintainers to push to their branches when they open a PR, because it increases the chance of your PR being merged. But still, it could be easy to miss, and I don’t think GitHub allows you to change it after the fact (something GitLab allows). So the only solutions I see there would be for GitHub to change.
This is not helpful here, but something to keep in mind.
Some forge software make the distinction between review and audit, where changes that were not reviewed can be audited after the fact.