refactor(branch-protection): change data structure to use pointer ins…

…tead of value At clients.BranchProtectionRule struct, changing RequiredPullRequestReviews to be a pointer instead of a struct value. This will allow the usage of the nil value of this structure to mean that we can't say if the repository requires reviews or not. Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
diogoteles08 · Nov 7, 2023 · 9d41826 · 9d41826
1 parent d7c9d95
commit 9d41826
Show file tree

Hide file tree

Showing 10 changed files with 66 additions and 47 deletions.
diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go
@@ -95,7 +95,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -115,7 +115,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -154,7 +154,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -189,7 +189,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -210,7 +210,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -245,7 +245,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -266,7 +266,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &trueVal,
 							Contexts:             []string{"foo"},
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &trueVal,
 							RequireCodeOwnerReviews:      &trueVal,
 							RequiredApprovingReviewCount: &oneVal,
@@ -302,7 +302,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -340,7 +340,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 							UpToDateBeforeMerge:  &falseVal,
 							Contexts:             nil,
 						},
-						RequiredPullRequestReviews: clients.PullRequestReviewRule{
+						RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 							DismissStaleReviews:          &falseVal,
 							RequireCodeOwnerReviews:      &falseVal,
 							RequiredApprovingReviewCount: &zeroVal,
@@ -358,9 +358,9 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         0,
-				NumberOfWarn:  4,
+				NumberOfWarn:  2,
 				NumberOfInfo:  0,
-				NumberOfDebug: 8,
+				NumberOfDebug: 10,
 			},
 			nonadmin:      true,
 			defaultBranch: main,

diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go
@@ -284,7 +284,8 @@ func nonAdminReviewProtection(branch *clients.BranchRef) (int, int) {
 	max := 0
 
 	max += 2
-	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
+	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
+		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil &&
 		*branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount > 0 {
 		// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
 		score += 2
@@ -330,7 +331,8 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
 		// If Scorecard is run with admin token, we can interprete GitHub's response to say
 		// if the branch requires PRs prior to code changes.
 		max++
-		if branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
+		if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
+			branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount != nil {
 			score++
 			info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name)
 		} else {
@@ -347,7 +349,8 @@ 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.DismissStaleReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
+		branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews != nil {
 		// Note: we don't inrecase max possible score for non-admin viewers.
 		max++
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
@@ -389,9 +392,11 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta
 	max++
 
 	// On this first check we exclude the case of PRs don't being required, covered on adminReviewProtection function
-	if !(isUserAdmin(branch) &&
+	if !(isUserAdmin(branch) && branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
 		branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount == nil) {
 		switch {
+		case branch.BranchProtectionRule.RequiredPullRequestReviews == nil:
+			debug(dl, log, "Unable to evaluate if PRs are required to make changes on branch '%s'", *branch.Name)
 		// If not running as admin, the nil value can both mean that no reviews are required or no PR are required,
 		// so here we assume no reviews are required.
 		case (!isUserAdmin(branch) &&
@@ -419,7 +424,8 @@ func codeownerBranchProtection(
 
 	log := branch.Protected != nil && *branch.Protected
 
-	if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
+	if branch.BranchProtectionRule.RequiredPullRequestReviews != nil &&
+		branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
 		switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
 		case true:
 			info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)

diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go
@@ -66,7 +66,7 @@ func TestIsBranchProtected(t *testing.T) {
 					RequireLinearHistory:    &falseVal,
 					EnforceAdmins:           &falseVal,
 					RequireLastPushApproval: &falseVal,
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -84,9 +84,9 @@ func TestIsBranchProtected(t *testing.T) {
 			expected: scut.TestReturn{
 				Error:         nil,
 				Score:         0,
-				NumberOfWarn:  2,
+				NumberOfWarn:  1,
 				NumberOfInfo:  0,
-				NumberOfDebug: 4,
+				NumberOfDebug: 5,
 			},
 			branch: &clients.BranchRef{
 				Name:      &branchVal,
@@ -106,7 +106,7 @@ func TestIsBranchProtected(t *testing.T) {
 				Name:      &branchVal,
 				Protected: &trueVal,
 				BranchProtectionRule: clients.BranchProtectionRule{
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -142,7 +142,7 @@ func TestIsBranchProtected(t *testing.T) {
 					RequireLinearHistory:    &falseVal,
 					AllowForcePushes:        &falseVal,
 					AllowDeletions:          &falseVal,
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -178,7 +178,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: nil,
@@ -209,7 +209,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -240,7 +240,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -271,7 +271,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: nil,
@@ -302,7 +302,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  nil,
 						Contexts:             nil,
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -333,7 +333,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -364,7 +364,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -396,7 +396,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -427,7 +427,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &falseVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &falseVal,
 						RequireCodeOwnerReviews:      &falseVal,
 						RequiredApprovingReviewCount: &zeroVal,
@@ -458,7 +458,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -489,7 +489,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,
@@ -521,7 +521,7 @@ func TestIsBranchProtected(t *testing.T) {
 						UpToDateBeforeMerge:  &trueVal,
 						Contexts:             []string{"foo"},
 					},
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &oneVal,

diff --git a/clients/branch.go b/clients/branch.go
@@ -23,7 +23,7 @@ type BranchRef struct {
 
 // BranchProtectionRule captures the settings enabled on a branch for security.
 type BranchProtectionRule struct {
-	RequiredPullRequestReviews PullRequestReviewRule
+	RequiredPullRequestReviews *PullRequestReviewRule
 	AllowDeletions             *bool
 	AllowForcePushes           *bool
 	RequireLinearHistory       *bool

diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go
@@ -404,6 +404,8 @@ func getBranchRefFrom(data *branch, rules []*repoRuleSet) *clients.BranchRef {
 	*branchRef.Protected = true
 	branchRule := &branchRef.BranchProtectionRule
 
+	branchRule.RequiredPullRequestReviews = new(clients.PullRequestReviewRule)
+
 	switch {
 	// All settings are available. This typically means
 	// scorecard is run with a token that has access
@@ -493,6 +495,9 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) {
 
 		translated.EnforceAdmins = initializedBoolRef(len(r.BypassActors.Nodes) == 0)
 
+		// Instatiate this struct as it will never be null on Repo Rules. We'll always know if PRs are required or not
+		translated.RequiredPullRequestReviews = new(clients.PullRequestReviewRule)
+
 		for _, rule := range r.Rules.Nodes {
 			switch rule.Type {
 			case ruleDeletion:
@@ -562,7 +567,7 @@ func mergeBranchProtectionRules(base, translated *clients.BranchProtectionRule)
 			readBoolPtr(base.RequireLinearHistory) || readBoolPtr(translated.RequireLinearHistory),
 		)
 	}
-	mergePullRequestReviews(&base.RequiredPullRequestReviews, &translated.RequiredPullRequestReviews)
+	mergePullRequestReviews(base.RequiredPullRequestReviews, translated.RequiredPullRequestReviews)
 	mergeCheckRules(&base.CheckRules, &translated.CheckRules)
 }
 
@@ -586,6 +591,10 @@ func mergeCheckRules(base, translated *clients.StatusChecksRule) {
 }
 
 func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) {
+	if base == nil {
+		base = new(clients.PullRequestReviewRule)
+	}
+
 	if translated.RequiredApprovingReviewCount != nil &&
 		(readIntPtr(translated.RequiredApprovingReviewCount) >= readIntPtr(base.RequiredApprovingReviewCount)) {
 		base.RequiredApprovingReviewCount = translated.RequiredApprovingReviewCount

diff --git a/clients/githubrepo/branches_test.go b/clients/githubrepo/branches_test.go
@@ -191,7 +191,7 @@ func Test_applyRepoRules(t *testing.T) {
 		name       string
 	}{
 		{
-			name:    "test if unchecked checkboxes have consistent values",
+			name:    "unchecked checkboxes have consistent values",
 			base:    &clients.BranchRef{},
 			ruleSet: ruleSet(),
 			expected: &clients.BranchRef{
@@ -207,7 +207,7 @@ func Test_applyRepoRules(t *testing.T) {
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: nil, // this checkbox is enabled only if require status checks
 					RequireLinearHistory:    &falseVal,
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						// nil values mean that this checkbox wasn't checked
 						RequiredApprovingReviewCount: nil,
 						DismissStaleReviews:          nil,
@@ -342,7 +342,7 @@ func Test_applyRepoRules(t *testing.T) {
 					EnforceAdmins:           &trueVal,
 					RequireLastPushApproval: &trueVal,
 					RequireLinearHistory:    &falseVal,
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						// DismissStaleReviews:          &trueVal,
 						RequiredApprovingReviewCount: &zeroVal,
 					},
@@ -371,7 +371,7 @@ func Test_applyRepoRules(t *testing.T) {
 					EnforceAdmins:           &trueVal,
 					RequireLinearHistory:    &falseVal,
 					RequireLastPushApproval: &trueVal,
-					RequiredPullRequestReviews: clients.PullRequestReviewRule{
+					RequiredPullRequestReviews: &clients.PullRequestReviewRule{
 						DismissStaleReviews:          &trueVal,
 						RequireCodeOwnerReviews:      &trueVal,
 						RequiredApprovingReviewCount: &twoVal,

diff --git a/clients/gitlabrepo/branches.go b/clients/gitlabrepo/branches.go
@@ -192,7 +192,7 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB
 		Contexts:             makeContextsFromResp(projectStatusChecks),
 	}
 
-	pullRequestReviewRule := clients.PullRequestReviewRule{
+	pullRequestReviewRule := &clients.PullRequestReviewRule{
 		DismissStaleReviews:     newTrue(),
 		RequireCodeOwnerReviews: &protectedBranch.CodeOwnerApprovalRequired,
 	}