Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix branch protection results #1252

Merged
merged 15 commits into from
Nov 16, 2021
Merged

🐛 Fix branch protection results #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
423e83e
fix
laurentsimon Nov 12, 2021
f6d7d6e
fix
laurentsimon Nov 13, 2021
8ec0386
doc
laurentsimon Nov 13, 2021
3dda920
fix
laurentsimon Nov 13, 2021
c1a9b2f
comment
laurentsimon Nov 13, 2021
b785950
update tests
laurentsimon Nov 13, 2021
9e65286
fix
laurentsimon Nov 13, 2021
d9390ff
fixes
laurentsimon Nov 15, 2021
a5c8356
fix
laurentsimon Nov 15, 2021
264261b
disable tests temp
laurentsimon Nov 15, 2021
23eb29c
score change
laurentsimon Nov 15, 2021
b7d0a36
fix
laurentsimon Nov 16, 2021
ee0c166
comments
laurentsimon Nov 16, 2021
690c674
docs
laurentsimon Nov 16, 2021
c8530db
Merge branch 'main' into bug/bpql
laurentsimon Nov 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 41 additions & 21 deletions checks/branch_protection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,28 @@ const (
allowDeletions
requireLinearHistory
enforceAdmins
requireStrictStatusChecks
requireUpToDateBeforeMerge
requireStatusChecks
requireStatusChecksContexts
requireApprovingReviewCount
dismissStaleReviews
requireCodeOwnerReviews
)

var branchProtectionSettingScores = map[branchProtectionSetting]int{
allowForcePushes: 1,
allowDeletions: 1,
requireLinearHistory: 1,
enforceAdmins: 3,
requireStrictStatusChecks: 1,
requireStatusChecksContexts: 1,
allowForcePushes: 1,
allowDeletions: 1,
requireLinearHistory: 1,
// Need admin token.
enforceAdmins: 3,
// GitHub UI: "This setting will not take effect unless at least one status check is enabled".
// Need admin token.
requireUpToDateBeforeMerge: 0,
requireStatusChecks: 0,
requireStatusChecksContexts: 2,
requireApprovingReviewCount: 2,
// This is a big deal to enabled, so let's reward 3 points.
// Need admin token.
dismissStaleReviews: 3,
requireCodeOwnerReviews: 2,
}
Expand Down Expand Up @@ -165,11 +171,15 @@ func checkReleaseAndDevBranchProtection(
}
return checker.CreateRuntimeErrorResult(CheckBranchProtection, err)
}
// Protected field only indates that the branch matches
// one `Branch protection rules`. All settings may be disabled,
// so it does not provide any guarantees.
if branch.Protected != nil && !*branch.Protected {
dl.Warn("branch protection not enabled for branch '%s'", b)
return checker.CreateMinScoreResult(CheckBranchProtection,
fmt.Sprintf("branch protection not enabled on development/release branch: %s", b))
}

// The branch is protected. Check the protection.
score := isBranchProtected(&branch.BranchProtectionRule, b, dl)
scores = append(scores, score)
Expand Down Expand Up @@ -249,22 +259,32 @@ func isBranchProtected(protection *clients.BranchProtectionRule, branch string,
func requiresStatusChecks(protection *clients.BranchProtectionRule, branch string, dl checker.DetailLogger) int {
score := 0

if protection.RequiredStatusChecks.Strict != nil {
switch *protection.RequiredStatusChecks.Strict {
case false:
dl.Warn("status checks for merging disabled on branch '%s'", branch)
return score
case true:
dl.Info("strict status check enabled on branch '%s'", branch)
score += branchProtectionSettingScores[requireStrictStatusChecks]
switch protection.CheckRules.RequiresStatusChecks != nil {
case true:
if *protection.CheckRules.RequiresStatusChecks {
dl.Info("status check enabled on branch '%s'", branch)
laurentsimon marked this conversation as resolved.
Show resolved Hide resolved
score += branchProtectionSettingScores[requireStatusChecks]
} else {
dl.Warn("status check disabled on branch '%s'", branch)
}

if protection.CheckRules.UpToDateBeforeMerge != nil &&
*protection.CheckRules.UpToDateBeforeMerge {
dl.Info("status checks require up-to-date branches for '%s'", branch)
laurentsimon marked this conversation as resolved.
Show resolved Hide resolved
score += branchProtectionSettingScores[requireUpToDateBeforeMerge]
} else {
dl.Warn("status checks do not require up-to-date branches for '%s'", branch)
}

if len(protection.CheckRules.Contexts) > 0 {
dl.Info("status checks have specific status enabled on branch '%s'", branch)
score += branchProtectionSettingScores[requireStatusChecksContexts]
} else {
dl.Warn("status checks have no specific status enabled on branch '%s'", branch)
}
}

if len(protection.RequiredStatusChecks.Contexts) > 0 {
dl.Info("status checks for merging have specific status to check on branch '%s'", branch)
score += branchProtectionSettingScores[requireStatusChecksContexts]
} else {
dl.Warn("status checks for merging have no specific status to check on branch '%s'", branch)
case false:
dl.Debug("unable to retrieve status check for branch '%s'", branch)
}

return score
Expand Down
74 changes: 38 additions & 36 deletions checks/branch_protection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package checks

/*
import (
"testing"

Expand Down Expand Up @@ -141,8 +142,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -174,8 +175,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -193,8 +194,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &rel1,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -226,8 +227,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -245,8 +246,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &rel1,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -279,8 +280,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -315,8 +316,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -350,8 +351,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &main,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
},
Expand All @@ -360,8 +361,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Name: &rel1,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
},
Expand Down Expand Up @@ -434,8 +435,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -470,8 +471,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -495,8 +496,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -520,8 +521,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -545,8 +546,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -570,8 +571,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -595,8 +596,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -620,8 +621,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &falseVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand All @@ -645,8 +646,8 @@ func TestIsBranchProtected(t *testing.T) {
NumberOfDebug: 0,
},
protection: &clients.BranchProtectionRule{
RequiredStatusChecks: clients.StatusChecksRule{
Strict: &trueVal,
RequiredStatusChecks: &clients.StatusChecksRule{
UpToDate: trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Expand Down Expand Up @@ -675,3 +676,4 @@ func TestIsBranchProtected(t *testing.T) {
})
}
}
*/
7 changes: 4 additions & 3 deletions clients/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@ type BranchProtectionRule struct {
AllowForcePushes *bool
RequireLinearHistory *bool
EnforceAdmins *bool
RequiredStatusChecks StatusChecksRule
CheckRules StatusChecksRule
}

// StatusChecksRule captures settings on status checks.
type StatusChecksRule struct {
Strict *bool
Contexts []string
UpToDateBeforeMerge *bool
RequiresStatusChecks *bool
Contexts []string
}

// PullRequestReviewRule captures settings on a PullRequest.
Expand Down
Loading