Skip to content

Commit

Permalink
Merge branch 'main' into bugfix-2302-codereview
Browse files Browse the repository at this point in the history
  • Loading branch information
@laurentsimon
laurentsimon authored Dec 19, 2022
2 parents 44d0e02 + 6c5d964 commit 6e1cf65
Show file tree
Hide file tree
Showing 14 changed files with 312 additions and 129 deletions.
3 changes: 2 additions & 1 deletion checker/raw_result.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ type WebhooksData struct {
// BranchProtectionsData contains the raw results
// for the Branch-Protection check.
type BranchProtectionsData struct {
Branches []clients.BranchRef
Branches []clients.BranchRef
CodeownersFiles []string
}

// Tool represents a tool.
Expand Down
70 changes: 39 additions & 31 deletions checks/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
defaultBranch string
releases []string
nonadmin bool
repoFiles []string
}{
{
name: "Nil release and main branch names",
Expand Down Expand Up @@ -134,7 +135,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
expected: scut.TestReturn{
Error: nil,
Score: 2,
NumberOfWarn: 6,
NumberOfWarn: 7,
NumberOfInfo: 2,
NumberOfDebug: 0,
},
Expand All @@ -158,10 +159,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
},
EnforceAdmins: &falseVal,
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &falseVal,
RequireLinearHistory: &falseVal,
RequireLastPushApproval: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
},
},
Expand All @@ -172,8 +174,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
expected: scut.TestReturn{
Error: nil,
Score: 2,
NumberOfWarn: 7,
NumberOfInfo: 9,
NumberOfWarn: 9,
NumberOfInfo: 10,
NumberOfDebug: 0,
},
defaultBranch: main,
Expand All @@ -192,10 +194,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
},
EnforceAdmins: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &trueVal,
RequireLastPushApproval: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
},
{
Expand All @@ -212,10 +215,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
},
EnforceAdmins: &falseVal,
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &falseVal,
RequireLastPushApproval: &falseVal,
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
},
},
Expand All @@ -226,8 +230,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
expected: scut.TestReturn{
Error: nil,
Score: 8,
NumberOfWarn: 2,
NumberOfInfo: 14,
NumberOfWarn: 4,
NumberOfInfo: 16,
NumberOfDebug: 0,
},
defaultBranch: main,
Expand All @@ -246,10 +250,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
},
EnforceAdmins: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &trueVal,
RequireLastPushApproval: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
},
{
Expand All @@ -266,10 +271,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
},
EnforceAdmins: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &trueVal,
RequireLastPushApproval: &trueVal,
RequireLinearHistory: &trueVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
},
},
Expand All @@ -280,7 +286,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
expected: scut.TestReturn{
Error: nil,
Score: 2,
NumberOfWarn: 6,
NumberOfWarn: 7,
NumberOfInfo: 2,
NumberOfDebug: 0,
},
Expand All @@ -301,10 +307,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
},
EnforceAdmins: &falseVal,
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
EnforceAdmins: &falseVal,
RequireLastPushApproval: &falseVal,
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
},
}, {
Name: &rel1,
Expand Down Expand Up @@ -353,7 +360,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
Score: 0,
NumberOfWarn: 4,
NumberOfInfo: 0,
NumberOfDebug: 6,
NumberOfDebug: 8,
},
nonadmin: true,
defaultBranch: main,
Expand Down Expand Up @@ -412,6 +419,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
DoAndReturn(func(b string) (*clients.BranchRef, error) {
return getBranch(tt.branches, b, tt.nonadmin), nil
}).AnyTimes()
mockRepoClient.EXPECT().ListFiles(gomock.Any()).AnyTimes().Return(tt.repoFiles, nil)
dl := scut.TestDetailLogger{}
req := checker.CheckRequest{
Dlogger: &dl,
Expand Down
39 changes: 29 additions & 10 deletions checks/evaluation/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func BranchProtection(name string, dl checker.DetailLogger,
score.scores.thoroughReview, score.maxes.thoroughReview = nonAdminThoroughReviewProtection(&b, dl)
// Do we want this?
score.scores.adminThoroughReview, score.maxes.adminThoroughReview = adminThoroughReviewProtection(&b, dl)
score.scores.codeownerReview, score.maxes.codeownerReview = codeownersBranchProtection(&b, dl)
score.scores.codeownerReview, score.maxes.codeownerReview = codeownerBranchProtection(&b, r.CodeownersFiles, dl)

scores = append(scores, score)
}
Expand Down Expand Up @@ -373,18 +373,31 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
// Only log information if the branch is protected.
log := branch.Protected != nil && *branch.Protected

if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil {
// Process UpToDateBeforeMerge value.
if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge == nil {
debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name)
} else {
// Note: `This setting will not take effect unless at least one status check is enabled`.
max++
switch *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge {
case true:
if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge {
info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name)
score++
default:
} else {
warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name)
}
}

// Process RequireLastPushApproval value.
if branch.BranchProtectionRule.RequireLastPushApproval == nil {
debug(dl, log, "unable to retrieve whether 'last push approval' is required to merge on branch '%s'", *branch.Name)
} else {
debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name)
max++
if *branch.BranchProtectionRule.RequireLastPushApproval {
info(dl, log, "'last push approval' enabled on branch '%s'", *branch.Name)
score++
} else {
warn(dl, log, "'last push approval' disabled on branch '%s'", *branch.Name)
}
}

return score, max
Expand All @@ -401,10 +414,10 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
max++
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
case true:
info(dl, log, "Stale review dismissal enabled on branch '%s'", *branch.Name)
info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name)
score++
case false:
warn(dl, log, "Stale review dismissal disabled on branch '%s'", *branch.Name)
warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name)
}
} else {
debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name)
Expand Down Expand Up @@ -436,7 +449,9 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
return score, max
}

func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogger) (int, int) {
func codeownerBranchProtection(
branch *clients.BranchRef, codeownersFiles []string, dl checker.DetailLogger,
) (int, int) {
score := 0
max := 1

Expand All @@ -446,7 +461,11 @@ func codeownersBranchProtection(branch *clients.BranchRef, dl checker.DetailLogg
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
case true:
info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
score++
if len(codeownersFiles) == 0 {
warn(dl, log, "codeowners branch protection is being ignored - but no codeowners file found in repo")
} else {
score++
}
default:
warn(dl, log, "codeowner review is not required on branch '%s'", *branch.Name)
}
Expand Down
Loading

0 comments on commit 6e1cf65

Please sign in to comment.