-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Protect master
branch to prevent accidents
#1073
Comments
It just felt unsafe to me that as soon as opened #1074, I could have immediately merged it. So for now I defaulted to safety and enabled a branch protection including mandatory CI checks, 1 PR approval, and no force-pushing. No problem if we want to turn this off, but defaulting to safety seemed best for now. |
jeffwidman
added a commit
that referenced
this issue
Apr 23, 2024
By default the `pull_request` trigger will run on every push to the PR branch. So we are wasting CI minutes and electricity by running our CI checks twice on every push to a PR branch. Instead, this makes checks run 1x per PR, and then also run on every merge to `master`, to ensure that `master` stays green. This latter check is normally useless, but occasionally if there's drift of some kind between when CI runs on a PR and when it's merged, then this can help identify the issue. A more common pattern is simply to only run on PR's, but given we haven't previously been enforcing "only merge via PR" (#1073) I thought might be best to keep checking `master` as well until that's changed. The one thing we stop doing with this change is checking on push to branches that aren't PR branches... ie, if a maintainer is working on testing something. But they may not even care about running CI on this branch, and if they do, it's easy to run the tests locally, or open a draft PR...So I don't see the point of preserving that behavior.
rbarrois
pushed a commit
that referenced
this issue
Apr 25, 2024
By default the `pull_request` trigger will run on every push to the PR branch. So we are wasting CI minutes and electricity by running our CI checks twice on every push to a PR branch. Instead, this makes checks run 1x per PR, and then also run on every merge to `master`, to ensure that `master` stays green. This latter check is normally useless, but occasionally if there's drift of some kind between when CI runs on a PR and when it's merged, then this can help identify the issue. A more common pattern is simply to only run on PR's, but given we haven't previously been enforcing "only merge via PR" (#1073) I thought might be best to keep checking `master` as well until that's changed. The one thing we stop doing with this change is checking on push to branches that aren't PR branches... ie, if a maintainer is working on testing something. But they may not even care about running CI on this branch, and if they do, it's easy to run the tests locally, or open a draft PR...So I don't see the point of preserving that behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I saw this on the home page:
I assume this was an accidental oversight... Or is there a reason this isn't currently enabled?
On other projects I help maintain, typically we setup the following:
master
is locked against pushes by everyone, only merge via PR.The text was updated successfully, but these errors were encountered: