Protect main from mistaken pushes

[max-sixty]

What is your issue?

Hi team — apologies but I mistakenly pushed to main. Less than a minute later I pushed another commit reverting the commit.

I'll check my git shortcuts tomorrow to ensure this can't happen by default — I thought I had the push remote set correctly, but possibly something doesn't use that.

Would we consider adding protection from pushes to main? (Am not blaming that setting tbc...)

(Will close this issue)

[jhamman]

Thanks for flagging this @max-sixty. We've all been there!

I support protecting the main branch.

[dcherian]

Sounds good to me too.

[Illviljan]

I thought this was already active? See #6833

[keewis]

it got lost at some point, I guess. I just enabled the "require pull requests" bit again (without any of the other requirements for that), but keep in mind that users with the repository "Admin" role can still override this (via force-push?).

[max-sixty]

Great, thanks a lot. (I'll close now :) )

[Illviljan]

Not sure if something changed but I managed to push directly to main today, 42fd510.

[keewis]

the branch protection should still be in place (including the "require pull request" rule), so I'm not sure why you were able to do that. I also checked the github changelog up until last year and didn't find anything, so really no clue why you were able to do that. I've enabled "Do not allow bypassing the above settings" just in case, but we might have to experiment with the rules.

[keewis]

I just tried creating a new branch, creating the exact same rules, and the push to the branch again. This was rejected, so I really have no idea why you were able to push to main.

Unless you were force-pushing? In that case you won't be able to do so anymore because of the "Do not allow bypassing the above settings" I enabled earlier.

[Illviljan]

hmm, I managed to push again, f85da8a.
Are you sure you enabled "Do not allow bypassing the above settings"?
Doesn't look like it's enabled on my side:

[keewis]

you're right, I forgot to click "save changes" 🤦‍♂️. It should be active now, and I can confirm that I can't push to main, not even with --force.

Also, for future tests: you can create empty commits by passing --allow-empty, so you won't need to revert the change again.

[max-sixty]

The problem with enabling this:

...is that we then can't merge if there are any failures on main.

...which includes the current state with mypy failures.

Ideally we would be able to disable pushing to main and allowing getting around the merge restriction (while maintaining the required checks). I don't think that's possible. Does anyone know any better?

(another option would be pinning dependencies so we don't get these long-running failures on main. But that either needs our own solution or to move to poetry from conda...)

---

I'll turn the restriction back on so as to not disturb anything, but I think it probably needs to be turned off.

[max-sixty]

I'll turn the restriction back on so as to not disturb anything, but I think it probably needs to be turned off.

I turned the restriction off gain, and this time I left it off, since I don't think there's a way around it while main is broken. We can also turn it on when we fix main.

...or if anyone has other ideas lmk!

[keewis]

yeah, probably. So that means that we're protected against accidental pushes, just not force pushes... though in general people should have a habit of double / triple checking what they're doing when it comes to force-pushing, and if you're not sure it's probably best to install some kind of pre-push hook that disallows pushes to origin/main or upstream/main.

[max-sixty]

yeah, probably. So that means that we're protected against accidental pushes, just not force pushes... though in general people should have a habit of double / triple checking what they're doing when it comes to force-pushing, and if you're not sure it's probably best to install some kind of pre-push hook that disallows pushes to origin/main or upstream/main.

No, this doesn't protect against normal pushes either...

Ideally we would be able to disable pushing to main and allowing getting around the merge restriction (while maintaining the required checks). I don't think that's possible. Does anyone know any better?

[headtr1ck]

We could simply add the no-commit-to-branch hook to our .pre-commit-config.yaml.

This would prevent commits on your local main (but only if you have pre-commit-hooks enabled, which I highly recommend!)

[max-sixty]

We could simply add the no-commit-to-branch hook to our .pre-commit-config.yaml.

This would prevent commits on your local main (but only if you have pre-commit-hooks enabled, which I highly recommend!)

Yes good idea!

(though re the recommendation — I find it slows down committing — I instead run pre-commit run -a when needed!)

[keewis]

you're right, though as far as I remember this used to work. Looks like the "branch protection" feature has been completely broken (even worse, without warning), and we have to use the new "ruleset" feature.

I just created one for main, which appears to block further pushes. We might have to fine-tune this, though.

[max-sixty]

I just created one for main, which appears to block further pushes. We might have to fine-tune this, though.

This seems to block merging anything!

#9248

[keewis]

okay, more details (hopefully correct this time): it appears branch protection rules don't apply by default to repository admins, which it seems you all are. So to resolve this using protection rules, we'd need two different sets: one for push protection that can't be bypassed by admins, and one for the required status checks that can be bypassed.

[keewis]

I've tried just that (and I still see the "bypass branch protections" for the merge button), but hopefully you can't push to main anymore.

Edit: I can confirm that is not possible anymore. So now we have a branch protection rule that makes sure nobody can push to main (and you can't bypass it), and a branch protection ruleset that requires status checks to pass, but which may be bypassed. Hopefully that allows us to resolve this?

[max-sixty]

Nice work @keewis re the two rulesets.

(it's odd from GH that it's kinda complicated to do the thing that seems very reasonable...)

I think we can close now?