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 <[email protected]>
Co-authored-by: Wes Todd <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Chris de Almeida <[email protected]>
Co-authored-by: Wes Todd <[email protected]>
Co-authored-by: Wes Todd <[email protected]>
Co-authored-by: Wes Todd <[email protected]>
| ### 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.
Related expressjs/security-wg#2
cc: @expressjs/express-tc @expressjs/security-wg