diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 69c0ddd35ff..9b5810399ae 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index 21352fb789a..cccf2e28977 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -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 } @@ -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 @@ -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 { @@ -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 @@ -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) @@ -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 +} diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index db25faf236b..86c34c3e5e6 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -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, @@ -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, @@ -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, @@ -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, @@ -175,7 +179,9 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &falseVal, Contexts: nil, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -202,7 +208,9 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -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, @@ -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, @@ -291,7 +301,6 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: nil, Contexts: nil, }, - RequiredPullRequestReviews: nil, }, }, }, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, @@ -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, diff --git a/clients/branch.go b/clients/branch.go index 4cd6b3d4217..9cabdbc3141 100644 --- a/clients/branch.go +++ b/clients/branch.go @@ -23,11 +23,7 @@ type BranchRef struct { // BranchProtectionRule captures the settings enabled on a branch for security. type BranchProtectionRule struct { - // The nil value of this struct can mean either: - // 1. we can't tell if PRs are required to make changes; or - // 2. we know PRs are not required. The first case happens when Scorecard is run without admin token on - // a repo that uses the old Branch Protection setting (not the new Repo Rules, that are always public). - RequiredPullRequestReviews *PullRequestReviewRule + RequiredPullRequestReviews PullRequestReviewRule AllowDeletions *bool AllowForcePushes *bool RequireLinearHistory *bool @@ -45,6 +41,7 @@ type StatusChecksRule struct { // PullRequestReviewRule captures settings on a PullRequest. type PullRequestReviewRule struct { + Required *bool // are PRs required RequiredApprovingReviewCount *int32 DismissStaleReviews *bool RequireCodeOwnerReviews *bool diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index b8ba122ef8e..4c42c07e1fc 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -327,6 +327,7 @@ func (handler *branchesHandler) getBranch(branch string) (*clients.BranchRef, er func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionRule) { copyBoolPtr(src.IsAdminEnforced, &dst.EnforceAdmins) copyBoolPtr(src.RequireLastPushApproval, &dst.RequireLastPushApproval) + copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) if src.RequiresStatusChecks != nil { copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks) // TODO(#3255): Update when GitHub GraphQL bug is fixed @@ -340,14 +341,13 @@ func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionR copyBoolPtr(&upToDateBeforeMerge, &dst.CheckRules.UpToDateBeforeMerge) } } - - // We're also checking if branch requires PRs because if it doesn't, - // the RequiredPullRequestReviews struct should be nil. - if src.DismissesStaleReviews != nil && branchRequiresPrs(src) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyBoolPtr(src.DismissesStaleReviews, &dst.RequiredPullRequestReviews.DismissStaleReviews) + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // these values report as &false when PRs aren't required, so if they're true then PRs are required + if valueOrZero(src.RequireLastPushApproval) || valueOrZero(src.DismissesStaleReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } } @@ -358,39 +358,36 @@ func copyNonAdminSettings(src interface{}, dst *clients.BranchProtectionRule) { copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) - // If branch doesn't require PRs to make changes, we let the struct RequiredPullRequestReviews point to nil - if branchRequiresPrs(v) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + // we always have the data to know if PRs are required + if dst.RequiredPullRequestReviews.Required == nil { + dst.RequiredPullRequestReviews.Required = asPtr(false) + } + // GitHub returns nil for RequiredApprovingReviewCount when PRs aren't required and non-nil when they are + // RequiresCodeOwnerReviews is &false even if PRs aren't required, so we need it to be true + if v.RequiredApprovingReviewCount != nil || valueOrZero(v.RequiresCodeOwnerReviews) { + dst.RequiredPullRequestReviews.Required = asPtr(true) } case *refUpdateRule: copyBoolPtr(v.AllowsDeletions, &dst.AllowDeletions) copyBoolPtr(v.AllowsForcePushes, &dst.AllowForcePushes) copyBoolPtr(v.RequiresLinearHistory, &dst.RequireLinearHistory) + copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) + copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts) // Evaluate if we have data to infer that the project requires PRs to make changes. If we don't have data, we let - // the struct RequiredPullRequestReviews as nil + // Required stay nil if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) { - if dst.RequiredPullRequestReviews == nil { - dst.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - } - copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.RequiredPullRequestReviews.RequiredApprovingReviewCount) - copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.RequiredPullRequestReviews.RequireCodeOwnerReviews) + dst.RequiredPullRequestReviews.Required = asPtr(true) } } } -func branchRequiresPrs(data *branchProtectionRule) bool { - return data.RequiredApprovingReviewCount != nil -} - func getDefaultBranchNameFrom(data *ruleSetData) string { if data == nil || data.Repository.DefaultBranchRef.Name == nil { return "" @@ -511,6 +508,9 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { AllowDeletions: asPtr(true), AllowForcePushes: asPtr(true), RequireLinearHistory: asPtr(false), + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: asPtr(false), + }, } translated.EnforceAdmins = asPtr(len(r.BypassActors.Nodes) == 0) @@ -534,8 +534,7 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) { } func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) { - base.RequiredPullRequestReviews = new(clients.PullRequestReviewRule) - + base.RequiredPullRequestReviews.Required = asPtr(true) base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval @@ -583,7 +582,7 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule) base.RequireLinearHistory = translated.RequireLinearHistory } mergeCheckRules(&base.CheckRules, &translated.CheckRules) - mergePullRequestReviews(&base.RequiredPullRequestReviews, translated.RequiredPullRequestReviews) + mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews) } func mergeCheckRules(base, translated *clients.StatusChecksRule) { @@ -601,26 +600,19 @@ func mergeCheckRules(base, translated *clients.StatusChecksRule) { } } -func mergePullRequestReviews(base **clients.PullRequestReviewRule, translated *clients.PullRequestReviewRule) { - switch { - case translated == nil: - // "translated" have nothing to be merged in base - return - case *base == nil: - // result would be "translated" itself - *base = translated - return +func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) { + if base.Required == nil || valueOrZero(translated.Required) { + base.Required = translated.Required } - - if (*base).RequiredApprovingReviewCount == nil || - valueOrZero((*base).RequiredApprovingReviewCount) < valueOrZero(translated.RequiredApprovingReviewCount) { - (*base).RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount + if base.RequiredApprovingReviewCount == nil || + valueOrZero(base.RequiredApprovingReviewCount) < valueOrZero(translated.RequiredApprovingReviewCount) { + base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount } - if (*base).DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) { - (*base).DismissStaleReviews = translated.DismissStaleReviews + if base.DismissStaleReviews == nil || valueOrZero(translated.DismissStaleReviews) { + base.DismissStaleReviews = translated.DismissStaleReviews } - if (*base).RequireCodeOwnerReviews == nil || valueOrZero(translated.RequireCodeOwnerReviews) { - (*base).RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews + if base.RequireCodeOwnerReviews == nil || valueOrZero(translated.RequireCodeOwnerReviews) { + base.RequireCodeOwnerReviews = translated.RequireCodeOwnerReviews } } diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go index d54b79ce750..e8467422449 100644 --- a/clients/githubrepo/branches_test.go +++ b/clients/githubrepo/branches_test.go @@ -206,10 +206,12 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: nil, Contexts: nil, }, - EnforceAdmins: &trueVal, - RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + EnforceAdmins: &trueVal, + RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -221,11 +223,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &trueVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -237,11 +241,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -258,11 +264,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -280,11 +288,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &falseVal, // Maintain: deletion enforces but forcepush does not + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -301,11 +311,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -317,11 +329,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &trueVal, - AllowForcePushes: &falseVal, - EnforceAdmins: &trueVal, - RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &trueVal, + AllowForcePushes: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &falseVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -333,11 +347,13 @@ func Test_applyRepoRules(t *testing.T) { }, expected: &clients.BranchRef{ BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &trueVal, - AllowForcePushes: &trueVal, - RequireLinearHistory: &trueVal, - EnforceAdmins: &trueVal, - RequiredPullRequestReviews: nil, + AllowDeletions: &trueVal, + AllowForcePushes: &trueVal, + RequireLinearHistory: &trueVal, + EnforceAdmins: &trueVal, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -362,7 +378,8 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, RequireLastPushApproval: &trueVal, RequireLinearHistory: &falseVal, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, RequiredApprovingReviewCount: &zeroVal, }, }, @@ -392,7 +409,8 @@ func Test_applyRepoRules(t *testing.T) { EnforceAdmins: &trueVal, RequireLinearHistory: &falseVal, RequireLastPushApproval: &trueVal, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &twoVal, @@ -429,7 +447,9 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + }, }, }, }, @@ -495,7 +515,8 @@ func Test_applyRepoRules(t *testing.T) { RequiresStatusChecks: &trueVal, Contexts: []string{"foo"}, }, - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &trueVal, RequiredApprovingReviewCount: &twoVal, DismissStaleReviews: &trueVal, RequireCodeOwnerReviews: &trueVal, @@ -556,7 +577,10 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) { RequiresStatusChecks: nil, Contexts: []string{}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + RequiredApprovingReviewCount: asPtr[int32](0), + RequireCodeOwnerReviews: &falseVal, + }, }, }, }, @@ -591,7 +615,12 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) { RequiresStatusChecks: &falseVal, Contexts: []string{}, }, - RequiredPullRequestReviews: nil, + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: &falseVal, + RequireCodeOwnerReviews: &falseVal, + DismissStaleReviews: &falseVal, + RequiredApprovingReviewCount: nil, + }, }, }, }, diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go index bd69d7e613e..f50f5e3cdb0 100644 --- a/clients/gitlabrepo/branches.go +++ b/clients/gitlabrepo/branches.go @@ -193,7 +193,8 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB Contexts: makeContextsFromResp(projectStatusChecks), } - pullRequestReviewRule := &clients.PullRequestReviewRule{ + pullRequestReviewRule := clients.PullRequestReviewRule{ + // TODO how do we know if they're required? DismissStaleReviews: newTrue(), RequireCodeOwnerReviews: &protectedBranch.CodeOwnerApprovalRequired, } diff --git a/cron/internal/format/json_raw_results.go b/cron/internal/format/json_raw_results.go index 9c19227c31e..1a2d6e64331 100644 --- a/cron/internal/format/json_raw_results.go +++ b/cron/internal/format/json_raw_results.go @@ -210,17 +210,15 @@ func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.Branch bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, + RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, + DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, + RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } - if v.BranchProtectionRule.RequiredPullRequestReviews != nil { - bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews - bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount - bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews - } } r.Results.BranchProtections = append(r.Results.BranchProtections, jsonBranchProtection{ Name: *v.Name, diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 7f2ef29d135..5d63cacfec8 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -712,17 +712,15 @@ func (r *jsonScorecardRawResult) addBranchProtectionRawResults(bp *checker.Branc bp = &jsonBranchProtectionSettings{ AllowsDeletions: v.BranchProtectionRule.AllowDeletions, AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes, + RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews, RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory, + DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews, EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins, RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks, RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge, + RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount, StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts, } - if v.BranchProtectionRule.RequiredPullRequestReviews != nil { - bp.DismissesStaleReviews = v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews - bp.RequiredApprovingReviewCount = v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount - bp.RequiresCodeOwnerReviews = v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews - } } branches = append(branches, jsonBranchProtection{ Name: *v.Name, diff --git a/pkg/json_raw_results_test.go b/pkg/json_raw_results_test.go index 098d6dfc974..261e6f81cee 100644 --- a/pkg/json_raw_results_test.go +++ b/pkg/json_raw_results_test.go @@ -1114,7 +1114,8 @@ func TestJsonScorecardRawResult(t *testing.T) { BranchProtectionRule: clients.BranchProtectionRule{ AllowDeletions: boolPtr(true), AllowForcePushes: boolPtr(false), - RequiredPullRequestReviews: &clients.PullRequestReviewRule{ + RequiredPullRequestReviews: clients.PullRequestReviewRule{ + Required: boolPtr(true), RequireCodeOwnerReviews: boolPtr(true), DismissStaleReviews: boolPtr(true), RequiredApprovingReviewCount: intPtr(2),