Proposing a couple of minor, but important protections, now that there are more cooks in the kitchen with permission to merge.
- Set
mainas a protected branch - Enable pull request reviews/approvals prior to merge
- No self-approvals, you can't submit a PR and approve your own submission
Org owners can technically bypass this, but it's bad form to do so without a good reason.
This prevents an "oops" situation, since we're currently only working on one branch. In the future, this can and should shift to a dev -> main pipeline for commits and versions, but above is fine, sane, and easy.
We discussed this in another thread somewhere, but I've lost track, and it's probably in one of the catch-alls that should be broken out into smaller targeted discussions.
Any concerns?
All reactions
-
👍 3
Replies: 3 comments 6 replies
|
Yes, agree 100% @fxd0h, can I ask that you please make these changes to main branch protection when you are ready. BTW: I made a tiny change to the README this morning... though didn't do a PR self-approval.
|
All reactions
I can make the changes, just wanted agreement before I did.
All reactions
-
👍 2
Agreed - thanks
All reactions
Agreed, table stakes for main branch management.
All reactions
Done. main now requires a PR with one approval and green fast-tests; force pushes and deletions are blocked, and owners can still technically bypass, per the bad-form clause. The bots stay advisory on purpose: a rate-limited or NEUTRAL bot check should not be able to freeze the queue.
All reactions
-
👍 1
Following up in this thread since it holds the original agreement. Two drifts since then.
The context we agreed to require was fast-tests. That workflow got renamed along the way, and the rules moved from classic protection into rulesets, so today the only check actually gating main is lint. Tests still run green on every PR, they just don't block anything anymore.
Meanwhile #685 landed the release process, and its release-note check is designed to block: the PR template ships the block empty so that filling it (or writing none) is a conscious call. While the check stays advisory, an unfilled block can merge anyway and someone has to catch it by hand at release time.
I'd like the required checks to be lint, tests and release-note, with the bots staying advisory as we set them. If no objections I can apply it to the ruleset like last time.
All reactions
OK, by me...
All reactions
Applied. The main-protection ruleset now requires lint, tests and release-note; bots stay advisory.
All reactions
-
👍 1
Yeah, it's needed. More friction, especially when someone is "in the zone" with PR reviews.