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.
No, I couldn’t, as I said already:
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.
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.
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
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.
Isn’t Nixpkgs too fast-moving for this? If I push to a NixOS/nixpkgs
branch, by the time CI has run master will likely have advanced.
When you enable status checks you have an option to enforce branches to be up-to-date with the base branch (second box in the screenshot below):
If you leave this box ticked, then yes, this would probably not work for nixpkgs. However, if you untick it, then you should be able to push a merge commit of your own branch (with the status check) with the base branch (e.g. master).
When you enable status checks you have an option to enforce branches
to be up-to-date with the base branch (second box in the screenshot
below):If you leave this box ticked, then yes, this would probably not work
for nixpkgs. However, if you untick it, then you should be able to
push a merge commit of your own branch (with the status check) with
the base branch (e.g. master).
Really? Because the merge commit wouldn’t itself have had tests run on
it (and you can change whatever you want in a merge commit).
I suspect that functionality is specific to the GitHub Merge Button.
I haven’t tested extensively all the possible combinations, so I cannot tell for sure, but what I can tell you with certainty is that in the Coq repository we have activated protected branches with required reviews (so not exactly the same as status checks) and that it does not prevent us from merging PRs on the command-line (using merge commits). The rest are indeed just assumptions based on my experience and would deserve to be tested.
if ofborg outages are rare enough
One more thing: if we require waiting for all the ofborg checks, then the main Hydra being stuck on Darwin only for a couple of days and therefore Darwin builds on ofborg timing out because the basic stuff is not in cache (I am not hypothesizing, only oversimplifying a recent event) blocks everything.
