Skip to content

Commit

Permalink
refactor(branch-protection): rename PullRequestReviewRule struct to P…
Browse files Browse the repository at this point in the history 
…ullRequestRule

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
  • Loading branch information
@diogoteles08
diogoteles08 committed Feb 16, 2024
1 parent e39802a commit 45f7a8c
Show file tree
Hide file tree
Showing 16 changed files with 113 additions and 113 deletions.
18 changes: 9 additions & 9 deletions checks/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand All @@ -112,7 +112,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -152,7 +152,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -188,7 +188,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand All @@ -210,7 +210,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -246,7 +246,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand All @@ -268,7 +268,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down Expand Up @@ -305,7 +305,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -344,7 +344,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down
14 changes: 7 additions & 7 deletions checks/evaluation/branch_protection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +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 valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount) > 0 {
if valueOrZero(branch.BranchProtectionRule.PullRequestRule.RequiredApprovingReviewCount) > 0 {
// We do not display anything here, it's done in nonAdminThoroughReviewProtection()
score += reviewerWeight
}
Expand Down Expand Up @@ -327,7 +327,7 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) (
}

max++
if valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.Required) {
if valueOrZero(branch.BranchProtectionRule.PullRequestRule.Required) {
score++
info(dl, log, "PRs are required in order to make changes on branch '%s'", *branch.Name)
} else {
Expand All @@ -345,10 +345,10 @@ 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.PullRequestRule.DismissStaleReviews != nil {
// Note: we don't increase max possible score for non-admin viewers.
max++
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews {
switch *branch.BranchProtectionRule.PullRequestRule.DismissStaleReviews {
case true:
info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name)
score++
Expand Down Expand Up @@ -386,7 +386,7 @@ func nonAdminThoroughReviewProtection(branch *clients.BranchRef, dl checker.Deta

max++

reviewers := valueOrZero(branch.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount)
reviewers := valueOrZero(branch.BranchProtectionRule.PullRequestRule.RequiredApprovingReviewCount)
if reviewers >= minReviews {
info(dl, log, "number of required reviewers is %d on branch '%s'", reviewers, *branch.Name)
score++
Expand All @@ -406,8 +406,8 @@ func codeownerBranchProtection(

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

if branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews != nil {
switch *branch.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews {
if branch.BranchProtectionRule.PullRequestRule.RequireCodeOwnerReviews != nil {
switch *branch.BranchProtectionRule.PullRequestRule.RequireCodeOwnerReviews {
case true:
info(dl, log, "codeowner review is required on branch '%s'", *branch.Name)
if len(codeownersFiles) == 0 {
Expand Down
30 changes: 15 additions & 15 deletions checks/evaluation/branch_protection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestIsBranchProtected(t *testing.T) {
RequireLinearHistory: &falseVal,
EnforceAdmins: &falseVal,
RequireLastPushApproval: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
CheckRules: clients.StatusChecksRule{
Expand Down Expand Up @@ -105,7 +105,7 @@ func TestIsBranchProtected(t *testing.T) {
Name: &branchVal,
Protected: &trueVal,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -142,7 +142,7 @@ func TestIsBranchProtected(t *testing.T) {
RequireLinearHistory: &falseVal,
AllowForcePushes: &falseVal,
AllowDeletions: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -179,7 +179,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand Down Expand Up @@ -208,7 +208,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand Down Expand Up @@ -237,7 +237,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down Expand Up @@ -269,7 +269,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -327,7 +327,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: nil,
Contexts: nil,
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: nil,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -359,7 +359,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -391,7 +391,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -424,7 +424,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -456,7 +456,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &falseVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &falseVal,
RequireCodeOwnerReviews: &falseVal,
Expand Down Expand Up @@ -488,7 +488,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down Expand Up @@ -520,7 +520,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down Expand Up @@ -553,7 +553,7 @@ func TestIsBranchProtected(t *testing.T) {
UpToDateBeforeMerge: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down
6 changes: 3 additions & 3 deletions clients/branch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type BranchRef struct {

// BranchProtectionRule captures the settings enabled on a branch for security.
type BranchProtectionRule struct {
RequiredPullRequestReviews PullRequestReviewRule
PullRequestRule PullRequestRule

Check failure on line 26 in clients/branch.go

View workflow job for this annotation

GitHub Actions / check-linter 

File is not `gofmt`-ed with `-s` (gofmt)
AllowDeletions *bool
AllowForcePushes *bool
RequireLinearHistory *bool
Expand All @@ -39,8 +39,8 @@ type StatusChecksRule struct {
Contexts []string
}

// PullRequestReviewRule captures settings on a PullRequest.
type PullRequestReviewRule struct {
// PullRequestRule captures settings on a PullRequest.
type PullRequestRule struct {
Required *bool // are PRs required
RequiredApprovingReviewCount *int32
DismissStaleReviews *bool
Expand Down
38 changes: 19 additions & 19 deletions clients/githubrepo/branches.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +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)
copyBoolPtr(src.DismissesStaleReviews, &dst.PullRequestRule.DismissStaleReviews)
if src.RequiresStatusChecks != nil {
copyBoolPtr(src.RequiresStatusChecks, &dst.CheckRules.RequiresStatusChecks)
// TODO(#3255): Update when GitHub GraphQL bug is fixed
Expand All @@ -342,12 +342,12 @@ func copyAdminSettings(src *branchProtectionRule, dst *clients.BranchProtectionR
}
}
// we always have the data to know if PRs are required
if dst.RequiredPullRequestReviews.Required == nil {
dst.RequiredPullRequestReviews.Required = asPtr(false)
if dst.PullRequestRule.Required == nil {
dst.PullRequestRule.Required = asPtr(false)

Check warning on line 346 in clients/githubrepo/branches.go

View check run for this annotation

Codecov / codecov/patch

clients/githubrepo/branches.go#L346 

Added line #L346 was not covered by tests
}
// 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)
dst.PullRequestRule.Required = asPtr(true)

Check warning on line 350 in clients/githubrepo/branches.go

View check run for this annotation

Codecov / codecov/patch

clients/githubrepo/branches.go#L350 

Added line #L350 was not covered by tests
}
}

Expand All @@ -358,32 +358,32 @@ 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)
copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.PullRequestRule.RequiredApprovingReviewCount)
copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.PullRequestRule.RequireCodeOwnerReviews)
copyStringSlice(v.RequiredStatusCheckContexts, &dst.CheckRules.Contexts)

// we always have the data to know if PRs are required
if dst.RequiredPullRequestReviews.Required == nil {
dst.RequiredPullRequestReviews.Required = asPtr(false)
if dst.PullRequestRule.Required == nil {
dst.PullRequestRule.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)
dst.PullRequestRule.Required = asPtr(true)

Check warning on line 372 in clients/githubrepo/branches.go

View check run for this annotation

Codecov / codecov/patch

clients/githubrepo/branches.go#L372 

Added line #L372 was not covered by tests
}

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)
copyInt32Ptr(v.RequiredApprovingReviewCount, &dst.PullRequestRule.RequiredApprovingReviewCount)
copyBoolPtr(v.RequiresCodeOwnerReviews, &dst.PullRequestRule.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
// Required stay nil
if valueOrZero(v.RequiredApprovingReviewCount) > 0 || valueOrZero(v.RequiresCodeOwnerReviews) {
dst.RequiredPullRequestReviews.Required = asPtr(true)
dst.PullRequestRule.Required = asPtr(true)

Check warning on line 386 in clients/githubrepo/branches.go

View check run for this annotation

Codecov / codecov/patch

clients/githubrepo/branches.go#L386 

Added line #L386 was not covered by tests
}
}
}
Expand Down Expand Up @@ -508,7 +508,7 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) {
AllowDeletions: asPtr(true),
AllowForcePushes: asPtr(true),
RequireLinearHistory: asPtr(false),
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: asPtr(false),
},
}
Expand All @@ -534,11 +534,11 @@ func applyRepoRules(branchRef *clients.BranchRef, rules []*repoRuleSet) {
}

func translatePullRequestRepoRule(base *clients.BranchProtectionRule, rule *repoRule) {
base.RequiredPullRequestReviews.Required = asPtr(true)
base.RequiredPullRequestReviews.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush
base.RequiredPullRequestReviews.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview
base.PullRequestRule.Required = asPtr(true)
base.PullRequestRule.DismissStaleReviews = rule.Parameters.PullRequestParameters.DismissStaleReviewsOnPush
base.PullRequestRule.RequireCodeOwnerReviews = rule.Parameters.PullRequestParameters.RequireCodeOwnerReview
base.RequireLastPushApproval = rule.Parameters.PullRequestParameters.RequireLastPushApproval
base.RequiredPullRequestReviews.RequiredApprovingReviewCount = rule.Parameters.PullRequestParameters.
base.PullRequestRule.RequiredApprovingReviewCount = rule.Parameters.PullRequestParameters.
RequiredApprovingReviewCount
}

Expand Down Expand Up @@ -582,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.PullRequestRule, &translated.PullRequestRule)
}

func mergeCheckRules(base, translated *clients.StatusChecksRule) {
Expand All @@ -600,7 +600,7 @@ func mergeCheckRules(base, translated *clients.StatusChecksRule) {
}
}

func mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) {
func mergePullRequestReviews(base, translated *clients.PullRequestRule) {
if base.Required == nil || valueOrZero(translated.Required) {
base.Required = translated.Required
}
Expand Down

0 comments on commit 45f7a8c

Please sign in to comment.