From d03c8cbb4356ef0bb4d2458181b013b0068e3936 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Tue, 12 Dec 2023 16:26:35 -0800 Subject: [PATCH] :bug: revert making RequiredPullRequestReviews a pointer (#3728) * revert the change which made RequiredPullRequestReviews a pointer While the current approach works with the tiered scoring, it wont work for probes or if we remove tiers. Making the struct nil to signal that PRs aren't required hides some of the data we do have. This is especially problematic for repo rules, where we can infer all settings by what we see or dont see. Signed-off-by: Spencer Schrock * add helper to deref pointers Signed-off-by: Spencer Schrock * clarify comments and keep code consistent Signed-off-by: Spencer Schrock --------- Signed-off-by: Spencer Schrock --- checks/branch_protection_test.go | 27 +++-- checks/evaluation/branch_protection.go | 42 ++++--- checks/evaluation/branch_protection_test.go | 61 ++++++---- clients/branch.go | 7 +- clients/githubrepo/branches.go | 82 ++++++-------- clients/githubrepo/branches_test.go | 119 ++++++++++++-------- clients/gitlabrepo/branches.go | 3 +- cron/internal/format/json_raw_results.go | 8 +- pkg/json_raw_results.go | 8 +- pkg/json_raw_results_test.go | 3 +- 10 files changed, 200 insertions(+), 160 deletions(-) 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),