So you’re advocating for not protecting the staging-next branch, correct?
1 Like
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)
1 Like
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.
4 Likes
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.
2 Likes
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
caprice32: 4.5.0 -> 4.6.0 by bignaux · Pull Request #82586 · NixOS/nixpkgs · GitHub 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.
1 Like
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.
5 Likes
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.
2 Likes
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.
4 Likes
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.
2 Likes
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.
2 Likes
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.
2 Likes
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.
There is another alternative for cases where you want to just clean up the commits before merging. Instead of directly pushing to master you could do a push to the contributors branch of the PR and then merge the clean version.
1 Like
No, I couldn’t, as I said already:
1 Like
In that case “not bothering” would have been a valid option as well. I don’t think its worth optimizing our process for that. It would be easy to overlook something while rebasing, creating an eval error on master and grinding everything else to a halt.
So I’d either just not bother or create a new PR, which really is not that much more effort than pushing to master.
2 Likes
In that case “not bothering” would have been a valid option as well. I
don’t think its worth optimizing our process for that. It would be
easy to overlook something while rebasing, creating an eval error on
master and grinding everything else to a halt.
Not if you have a pre-push hook, for example. We could have one of
those that runs a smoke test, or even just the OfBorg meta checks, and
say that if you’re going to push directly you should install this hook.
It does. And the box is ticked by default anyway. If it was unticked, it was the result of a specific action from the PR author (another, infrequent case, is when opening a PR from a branch that’s not in your own fork: in this case you cannot grant maintainers permission to push to the branch).
Umm, I’m not sure would “It does” is a response to in context to my response. Do you mean it does allow you to change it post PR open? (if so, I’ll probably add this to the PR template right away)
Yes.
In the right-hand bar.
1 Like
Basic branch protection rules (what is activated right now) only prevent force-pushing and branch deletion.
Additionally, GitHub allows enforcing additional checks:
-
Some status checks can be enforced. In this case, it does not force committers to open a PR first. You can also push to a branch (on the main repo) to have the test run, provided that you can have it run on commits outside PRs. Maybe that’s not the case of ofBorg checks.
-
Require approval. In this case, opening a PR is required and it must be approved by someone else than the PR author (with write access). In the current situation of nixpkgs, this solution would seem highly unrealistic.
Once you activate such checks, the only ones that can bypass them are repository admins.
Note however, that status checks are very flexible as you can use them to check anything. Example here: GitHub - Zimmi48/certified-origin: Limit to one pusher thanks to certified-origin GitHub status check
2 Likes