docs: add ADR for branch protection #391
Conversation
UlisesGascon
requested review from
wesleytodd,
blakeembrey,
sheplu,
jonchurch and
ctcpip
| ### Phase 2: Harden Governance | ||
|
|
||
| - Periodic review of admin bypass usage using the Scorecard Monitor ([ref](https://github.com/expressjs/security-wg/pull/70)) | ||
| - Enforce admin bypass documentation for emergency changes |
There was a problem hiding this comment.
can we split this off into a separate decision?
There was a problem hiding this comment.
I added a comment about adding more valid reasons above, but seeing this comment I think I agree that we should make that an amendment onto this larger ADR. For a while our CI was broken and the only way I could merge things was bypassing the protections. I think it is unreasonable to limit ourselves like this until we have automated releases since we can always fix the history if a mistake is made.
There was a problem hiding this comment.
Sounds good. No problem to split... what do we want to split only the Implementation or just the bypass rules?
There was a problem hiding this comment.
Chris was speaking specifically about keeping the Branch Protection ADR as its own, and a score card ADR as a separate decision.
There was a problem hiding this comment.
I think all the other changes were made, and this one only says "enforce" the rules we document above. I think that means we can mark it resolve. I am going to and change to approve, but as it was @ctcpip's comment I am very much alright with re-opening it to continue discussion if necessary.
There was a problem hiding this comment.
Most of my comments are nit picks, but I do agree with @ctcpip that we should spin out the bypass stuff into a follow up. It can even be an edit to this after we land IMO, but I like the main content of this so it would be unfortunate to hold that up because we need to discuss one small part.
| - Require all status checks to pass before merging | ||
|
|
||
| Optional rules: | ||
| - Enforce linear history (disable merge commits, allow squash/rebase) |
There was a problem hiding this comment.
Big fan of this, and I think we also had earlier discussion about this where we all generally agreed on it.
There was a problem hiding this comment.
True, We can do it in a separate ADR?
There was a problem hiding this comment.
I think we could leave it here as optional and then open a PR to move it to required and discuss that?
There was a problem hiding this comment.
unfortunately tho there's issues with this; linear history is good, but github's rebasemerge/squashmerge is bad.
There was a problem hiding this comment.
funny -- we just ran into this earlier today, where I had to do a git CLI commit undo, rebase, etc. I can go either way on the branch setting itself, but there will be times it will need to be disabled to avoid messy and risky rebase work
| ### Phase 2: Harden Governance | ||
|
|
||
| - Periodic review of admin bypass usage using the Scorecard Monitor ([ref](https://github.com/expressjs/security-wg/pull/70)) | ||
| - Enforce admin bypass documentation for emergency changes |
There was a problem hiding this comment.
I added a comment about adding more valid reasons above, but seeing this comment I think I agree that we should make that an amendment onto this larger ADR. For a while our CI was broken and the only way I could merge things was bypassing the protections. I think it is unreasonable to limit ourselves like this until we have automated releases since we can always fix the history if a mistake is made.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Wes Todd <wes@wesleytodd.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Chris de Almeida <ctcpip@users.noreply.github.com>
Co-authored-by: Wes Todd <wes@wesleytodd.com>
Co-authored-by: Wes Todd <wes@wesleytodd.com>
Co-authored-by: Wes Todd <wes@wesleytodd.com>
| ### Phase 2: Harden Governance | ||
|
|
||
| - Periodic review of admin bypass usage using the Scorecard Monitor ([ref](https://github.com/expressjs/security-wg/pull/70)) | ||
| - Enforce admin bypass documentation for emergency changes |
There was a problem hiding this comment.
I think all the other changes were made, and this one only says "enforce" the rules we document above. I think that means we can mark it resolve. I am going to and change to approve, but as it was @ctcpip's comment I am very much alright with re-opening it to continue discussion if necessary.
|
So... I plan to land this next week. Please add your reviews or blockers :) |
|
|
||
| Optional rules: | ||
| - Enforce linear history (disable merge commits, allow squash/rebase) | ||
| - Restrict who can push directly to the protected branches to people who can release (captains, TC, etc) |
There was a problem hiding this comment.
Can you help me understand this optional rule and how it interacts with the exception below?
From my understanding enabling this would allow the people who can release to permanently bypass the branch protection rules, rather than temporarily. It'd be simpler to have everything follow the same process to avoid any confusion when it comes down to this.
|
|
||
| - Identify all active repositories | ||
| - Notify repo maintainers of the change | ||
| - Apply default protection rules using GitHub API or organization policies |
There was a problem hiding this comment.
Can you document the API you use so people can re-use/update it over time? 🙏
| @@ -0,0 +1,109 @@ | |||
| # ADR 367: Enforce Branch Protection by Default Across All Repositories | |||
Related expressjs/security-wg#2
cc: @expressjs/express-tc @expressjs/security-wg