search

pydata / xarray

N-D labeled arrays and datasets in Python
https://xarray.dev
Apache License 2.0
3.56k stars 1.07k forks source link

Protect `main` from mistaken pushes #8209

Open max-sixty opened 12 months ago

max-sixty commented 12 months ago

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 commented 12 months ago

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

I support protecting the main branch.

dcherian commented 12 months ago

Sounds good to me too.

Illviljan commented 12 months ago

I thought this was already active? See https://github.com/pydata/xarray/issues/6833

keewis commented 12 months ago

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 commented 12 months ago

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

Illviljan commented 2 months ago

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

keewis commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

keewis commented 2 months ago

you're right, I forgot to click "save changes" :man_facepalming:. 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 commented 2 months ago

The problem with enabling this:

image

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

image

...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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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!

image

https://github.com/pydata/xarray/pull/9248

keewis commented 2 months ago

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 commented 2 months ago

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 commented 2 days ago

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?