From 95837bf61b8dbb9d01290d0a1ef354cb7243f2db Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 19 Jan 2024 11:02:10 -0500 Subject: [PATCH 1/5] Add ADR for GitHub branch protection --- doc/adr/0004-protect-main-branch.md | 61 +++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 doc/adr/0004-protect-main-branch.md diff --git a/doc/adr/0004-protect-main-branch.md b/doc/adr/0004-protect-main-branch.md new file mode 100644 index 000000000..94bfd0861 --- /dev/null +++ b/doc/adr/0004-protect-main-branch.md @@ -0,0 +1,61 @@ +# 4. Protect main branch + +Date: 2024-01-18 + +Authors: Josh Brown + +## Status + +Accepted + +## Context & Problem Statement + +Without branch protections, it’s too easy to merge PRs to the `main` branch without running checks or getting the necessary approvals. + +## Considered Options + +### Leave the `main` branch unprotected + +**Pros**: + +- It's easy. Leaving the main branch unprotected requires zero effort. + +**Cons**: + +- PRs can be merged easily without approval. +- PRs can be merged easily without passing checks. +- It's easy to push directly to `main` without creating a PR and getting approval(s) and PR checks. + +### Protect the `main` branch + +To do this, we propose add the following rules [in GitHub](https://github.com/planetary-social/nos/settings/rules): + +- Restrict deletions. Only allow users with bypass permissions to delete matching refs. +- Require a pull request before merging. Require all commits be made to a non-target branch and submitted via a pull request before they can be merged. +- Required approvals: 1. The number of approving reviews that are required before a pull request can be merged. +- Require status checks to pass: + - Unit tests + - swift_lint + - Check CHANGELOG + - license/cla +- Block force pushes. Prevent users with push access from force pushing to refs. + +**Pros**: + +Protecting the main branch promotes the behavior we want by making it harder to do the wrong things: + +- It’s harder to bypass PR checks we’ve determined are required to merge. +- It’s harder to merge PRs without the required approval. + +**Cons**: + +The pros are also cons, at least in the event of an emergency: + +- It’s harder to bypass PR checks. +- It’s harder to merge PRs without the required approval. + +Note that it’s only *slightly harder* -- not impossible -- to bypass PR checks and the required approval. There’s always a manual override that admins and maintainers can use. + +## Decision + +We’ve agreed to protect the `main` branch, as this encourages us to pass the checks and get the required PR approval. We’ll allow admins and maintainers to bypass checks and approvals, which requires an extra step when merging a PR. From 65a7845db811f4424169925c40918c7b86b1f308 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 19 Jan 2024 12:08:10 -0500 Subject: [PATCH 2/5] update link; fix grammar --- doc/adr/0004-protect-main-branch.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/adr/0004-protect-main-branch.md b/doc/adr/0004-protect-main-branch.md index 94bfd0861..424e2bbdc 100644 --- a/doc/adr/0004-protect-main-branch.md +++ b/doc/adr/0004-protect-main-branch.md @@ -28,7 +28,7 @@ Without branch protections, it’s too easy to merge PRs to the `main` branch wi ### Protect the `main` branch -To do this, we propose add the following rules [in GitHub](https://github.com/planetary-social/nos/settings/rules): +To do this, we propose adding the following branch protection rules [in GitHub](https://github.com/planetary-social/nos/settings/branches): - Restrict deletions. Only allow users with bypass permissions to delete matching refs. - Require a pull request before merging. Require all commits be made to a non-target branch and submitted via a pull request before they can be merged. From 6df2a8eecdef975aa1704d0e83ca27d78cba3b80 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 19 Jan 2024 12:14:59 -0500 Subject: [PATCH 3/5] further clarification and detail --- doc/adr/0004-protect-main-branch.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/adr/0004-protect-main-branch.md b/doc/adr/0004-protect-main-branch.md index 424e2bbdc..6129039c9 100644 --- a/doc/adr/0004-protect-main-branch.md +++ b/doc/adr/0004-protect-main-branch.md @@ -28,7 +28,7 @@ Without branch protections, it’s too easy to merge PRs to the `main` branch wi ### Protect the `main` branch -To do this, we propose adding the following branch protection rules [in GitHub](https://github.com/planetary-social/nos/settings/branches): +To do this, we propose adding the following branch protection rules [in GitHub](https://github.com/planetary-social/nos/settings/rules): - Restrict deletions. Only allow users with bypass permissions to delete matching refs. - Require a pull request before merging. Require all commits be made to a non-target branch and submitted via a pull request before they can be merged. @@ -40,6 +40,8 @@ To do this, we propose adding the following branch protection rules [in GitHub]( - license/cla - Block force pushes. Prevent users with push access from force pushing to refs. +We’d use the Rulesets feature rather than the Branch protection rules for the benefits [outlined by GitHub](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets#about-rulesets-protected-branches-and-protected-tags). Specifically: “Anyone with read access to a repository can view the active rulesets for the repository. This means a developer can understand why they have hit a rule, or an auditor can check the security constraints for the repository, without requiring admin access to the repository.” + **Pros**: Protecting the main branch promotes the behavior we want by making it harder to do the wrong things: @@ -58,4 +60,4 @@ Note that it’s only *slightly harder* -- not impossible -- to bypass PR checks ## Decision -We’ve agreed to protect the `main` branch, as this encourages us to pass the checks and get the required PR approval. We’ll allow admins and maintainers to bypass checks and approvals, which requires an extra step when merging a PR. +We’ve agreed to protect the `main` branch using Rulesets, as this encourages us to pass the checks and get the required PR approval. We’ll allow admins and maintainers to bypass checks and approvals, which requires an extra step when merging a PR. From aea1d2c3255e148dd46785c48dae274a9446d0da Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Mon, 22 Jan 2024 10:12:56 -0500 Subject: [PATCH 4/5] rename to ADR #5 from #4 --- ...{0004-protect-main-branch.md => 0005-protect-main-branch.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename doc/adr/{0004-protect-main-branch.md => 0005-protect-main-branch.md} (99%) diff --git a/doc/adr/0004-protect-main-branch.md b/doc/adr/0005-protect-main-branch.md similarity index 99% rename from doc/adr/0004-protect-main-branch.md rename to doc/adr/0005-protect-main-branch.md index 6129039c9..a9ec463cf 100644 --- a/doc/adr/0004-protect-main-branch.md +++ b/doc/adr/0005-protect-main-branch.md @@ -1,4 +1,4 @@ -# 4. Protect main branch +# 5. Protect main branch Date: 2024-01-18 From bd563245c17eeedc8361a21d9627d98bc8e436c3 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Mon, 22 Jan 2024 10:15:34 -0500 Subject: [PATCH 5/5] convert tabs to spaces --- doc/adr/0005-protect-main-branch.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/adr/0005-protect-main-branch.md b/doc/adr/0005-protect-main-branch.md index a9ec463cf..6cf53a134 100644 --- a/doc/adr/0005-protect-main-branch.md +++ b/doc/adr/0005-protect-main-branch.md @@ -34,10 +34,10 @@ To do this, we propose adding the following branch protection rules [in GitHub]( - Require a pull request before merging. Require all commits be made to a non-target branch and submitted via a pull request before they can be merged. - Required approvals: 1. The number of approving reviews that are required before a pull request can be merged. - Require status checks to pass: - - Unit tests - - swift_lint - - Check CHANGELOG - - license/cla + - Unit tests + - swift_lint + - Check CHANGELOG + - license/cla - Block force pushes. Prevent users with push access from force pushing to refs. We’d use the Rulesets feature rather than the Branch protection rules for the benefits [outlined by GitHub](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets#about-rulesets-protected-branches-and-protected-tags). Specifically: “Anyone with read access to a repository can view the active rulesets for the repository. This means a developer can understand why they have hit a rule, or an auditor can check the security constraints for the repository, without requiring admin access to the repository.”