Skip to content

Commit

Permalink
Merge branch 'main' into cleanup/dave/typos
Browse files Browse the repository at this point in the history
  • Loading branch information
justaugustus authored Dec 13, 2023
2 parents e664d85 + 2c20be0 commit 236e994
Show file tree
Hide file tree
Showing 22 changed files with 1,409 additions and 314 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/stale.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ jobs:
repo-token: ${{ secrets.GITHUB_TOKEN }}
stale-issue-message: 'This issue is stale because it has been open for 60 days with no activity.'
stale-pr-message: 'This pull request is stale because it has been open for 10 days with no activity'
exempt-issue-labels: 'priority,bug,good first issue'
exempt-issue-labels: 'priority,bug,good first issue,backlog,help wanted'
exempt-issue-milestones: 'Structured results'
exempt-pr-labels: 'awaiting-approval,work-in-progress'
days-before-pr-stale: '10'
Expand Down
27 changes: 18 additions & 9 deletions checks/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand All @@ -112,7 +113,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -151,7 +153,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -186,7 +189,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand All @@ -207,7 +211,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -242,7 +247,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand All @@ -263,7 +269,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down Expand Up @@ -299,7 +306,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -337,7 +345,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down
42 changes: 20 additions & 22 deletions checks/evaluation/branch_protection.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,7 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) {
// Having at least 1 reviewer is twice as important as the other Tier 2 requirements.
const reviewerWeight = 2
max += reviewerWeight
if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 {
if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) > 0 {
// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
score += reviewerWeight
}
Expand Down Expand Up @@ -329,15 +327,13 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
}

max++
if branch.BranchProtectionRule.RequiredPullRequestReviews != nil {
if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.Required) {
score++
info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name)
} else {
warn(dl, log, "PRs are not required to make changes on branch '%s'; or we don't have data to detect it."+
"If you think it might be the latter, make sure to run Scorecard with a PAT or use Repo "+
"Rules (that are always public) instead of Branch Protection settings", *branch.Name)
// Warning it here because since `RequiredPullRequestReviews` is nil, we won't check reviewers count afterwards
warn(dl, log, "No reviewers required to merge changes on branch '%s'", *branch.Name)
}

return score, max
Expand All @@ -349,8 +345,7 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL
// Only log information if the branch is protected.
log := branch.Protected != nil && *branch.Protected

if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
if branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
// Note: we don't increase max possible score for non-admin viewers.
max++
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
Expand Down Expand Up @@ -391,18 +386,13 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta

max++

// On this first check we exclude the case where PRs are not required to make changes,
// because it's already covered on adminReviewProtection function.
if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
if *branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews {
info(dl, log, "number of required reviewers is %d on branch '%s'",
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name)
score++
} else {
warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, *branch.Name, minReviews)
}
reviewers := valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount)
if reviewers >= minReviews {
info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name)
score++
} else {
warn(dl, log, "number of required reviewers is %d on branch '%s', while the ideal suggested is %d",
reviewers, *branch.Name, minReviews)
}

return score, max
Expand All @@ -416,8 +406,7 @@ func codeownerBranchProtection(

log := branch.Protected != nil && *branch.Protected

if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
case true:
info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
Expand All @@ -433,3 +422,12 @@ func codeownerBranchProtection(

return score, max
}

// returns the pointer's value if it exists, the type's zero-value otherwise.
func valueOrZero[T any](ptr *T) T {
if ptr == nil {
var zero T
return zero
}
return *ptr
}
61 changes: 39 additions & 22 deletions checks/evaluation/branch_protection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestIsBranchProtected(t *testing.T) {
expected scut.TestReturn
}{
{
name: "Configs as they are right after creating new Branch Protection setting",
name: "GitHub default settings",
expected: scut.TestReturn{
Error: nil,
Score: 3,
Expand All @@ -62,12 +62,14 @@ func TestIsBranchProtected(t *testing.T) {
Name: &branchVal,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
AllowDeletions: &falseVal,
AllowForcePushes: &falseVal,
RequireLinearHistory: &falseVal,
EnforceAdmins: &falseVal,
RequireLastPushApproval: &falseVal,
RequiredPullRequestReviews: nil,
AllowDeletions: &falseVal,
AllowForcePushes: &falseVal,
RequireLinearHistory: &falseVal,
EnforceAdmins: &falseVal,
RequireLastPushApproval: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &falseVal,
},
CheckRules: clients.StatusChecksRule{
RequiresStatusChecks: &trueVal,
Contexts: nil,
Expand Down Expand Up @@ -103,7 +105,8 @@ func TestIsBranchProtected(t *testing.T) {
Name: &branchVal,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -139,7 +142,8 @@ func TestIsBranchProtected(t *testing.T) {
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -175,7 +179,9 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: nil,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &falseVal,
},
},
},
},
Expand All @@ -202,7 +208,9 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: nil,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &falseVal,
},
},
},
},
Expand All @@ -229,7 +237,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -260,7 +269,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down Expand Up @@ -291,7 +301,6 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: nil,
Contexts: nil,
},
RequiredPullRequestReviews: nil,
},
},
},
Expand All @@ -318,7 +327,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: nil,
Contexts: nil,
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: nil,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down Expand Up @@ -349,7 +359,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -380,7 +391,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -412,7 +424,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -443,7 +456,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
RequiredApprovingReviewCount: &zeroVal,
Expand Down Expand Up @@ -474,7 +488,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down Expand Up @@ -505,7 +520,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down Expand Up @@ -537,7 +553,8 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: &clients.PullRequestReviewRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
RequiredApprovingReviewCount: &oneVal,
Expand Down
Loading

0 comments on commit 236e994

Please sign in to comment.