Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚠️ refactor: rename fields on Branch Protection Pull Request rules #3879

Merged
merged 2 commits into from
Mar 25, 2024
Merged

⚠️ refactor: rename fields on Branch Protection Pull Request rules #3879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd0231b
force push: re-apply refactor after huge changes on main
diogoteles08 Mar 21, 2024
ec9fa04
Merge branch 'main' into refactor/renaming-structure-on-PRs-branch-pr…
diogoteles08 Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 9 additions & 9 deletions checks/branch_protection_test.go
View file
Open in desktop
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
18 changes: 9 additions & 9 deletions clients/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ type BranchRef struct {

// BranchProtectionRule captures the settings enabled on a branch for security.
type BranchProtectionRule struct {
RequiredPullRequestReviews PullRequestReviewRule
AllowDeletions *bool
AllowForcePushes *bool
RequireLinearHistory *bool
EnforceAdmins *bool
RequireLastPushApproval *bool
CheckRules StatusChecksRule
PullRequestRule PullRequestRule
AllowDeletions *bool
AllowForcePushes *bool
RequireLinearHistory *bool
EnforceAdmins *bool
RequireLastPushApproval *bool
CheckRules StatusChecksRule
}

// StatusChecksRule captures settings on status checks.
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@
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 @@
}
}
// 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 @@
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 @@
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 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 @@
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 mergePullRequestReviews(base, translated *clients.PullRequestReviewRule) {
func mergePullRequestReviews(base, translated *clients.PullRequestRule) {
if base.Required == nil || valueOrZero(translated.Required) {
base.Required = translated.Required
}
Expand Down
28 changes: 14 additions & 14 deletions clients/githubrepo/branches_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,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{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -227,7 +227,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &trueVal,
RequireLinearHistory: &falseVal,
EnforceAdmins: &trueVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -245,7 +245,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &trueVal,
RequireLinearHistory: &falseVal,
EnforceAdmins: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -268,7 +268,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &falseVal,
EnforceAdmins: &falseVal, // Downgrade: deletion does not enforce admins
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -292,7 +292,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &falseVal,
EnforceAdmins: &falseVal, // Maintain: deletion enforces but force-push does not
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -315,7 +315,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &falseVal,
EnforceAdmins: &trueVal, // Maintain: base and rule are equal strictness
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -333,7 +333,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &falseVal,
EnforceAdmins: &trueVal,
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -351,7 +351,7 @@ func Test_applyRepoRules(t *testing.T) {
AllowForcePushes: &trueVal,
RequireLinearHistory: &trueVal,
EnforceAdmins: &trueVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand All @@ -378,7 +378,7 @@ func Test_applyRepoRules(t *testing.T) {
EnforceAdmins: &trueVal,
RequireLastPushApproval: &trueVal,
RequireLinearHistory: &falseVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
RequiredApprovingReviewCount: &zeroVal,
},
Expand Down Expand Up @@ -409,7 +409,7 @@ func Test_applyRepoRules(t *testing.T) {
EnforceAdmins: &trueVal,
RequireLinearHistory: &falseVal,
RequireLastPushApproval: &trueVal,
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
DismissStaleReviews: &trueVal,
RequireCodeOwnerReviews: &trueVal,
Expand Down Expand Up @@ -447,7 +447,7 @@ func Test_applyRepoRules(t *testing.T) {
RequiresStatusChecks: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
},
},
Expand Down Expand Up @@ -515,7 +515,7 @@ func Test_applyRepoRules(t *testing.T) {
RequiresStatusChecks: &trueVal,
Contexts: []string{"foo"},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &trueVal,
RequiredApprovingReviewCount: &twoVal,
DismissStaleReviews: &trueVal,
Expand Down Expand Up @@ -577,7 +577,7 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) {
RequiresStatusChecks: nil,
Contexts: []string{},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
RequiredApprovingReviewCount: asPtr[int32](0),
RequireCodeOwnerReviews: &falseVal,
},
Expand Down Expand Up @@ -615,7 +615,7 @@ func Test_translationFromGithubAPIBranchProtectionData(t *testing.T) {
RequiresStatusChecks: &falseVal,
Contexts: []string{},
},
RequiredPullRequestReviews: clients.PullRequestReviewRule{
PullRequestRule: clients.PullRequestRule{
Required: &falseVal,
RequireCodeOwnerReviews: &falseVal,
DismissStaleReviews: &falseVal,
Expand Down
12 changes: 6 additions & 6 deletions clients/gitlabrepo/branches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB
Contexts: makeContextsFromResp(projectStatusChecks),
}

pullRequestReviewRule := clients.PullRequestReviewRule{
pullRequestReviewRule := clients.PullRequestRule{
// TODO how do we know if they're required?
DismissStaleReviews: newTrue(),
RequireCodeOwnerReviews: &protectedBranch.CodeOwnerApprovalRequired,
Expand All @@ -208,11 +208,11 @@ func makeBranchRefFrom(branch *gitlab.Branch, protectedBranch *gitlab.ProtectedB
Name: &branch.Name,
Protected: &branch.Protected,
BranchProtectionRule: clients.BranchProtectionRule{
RequiredPullRequestReviews: pullRequestReviewRule,
AllowDeletions: newFalse(),
AllowForcePushes: &protectedBranch.AllowForcePush,
EnforceAdmins: newTrue(),
CheckRules: statusChecksRule,
PullRequestRule: pullRequestReviewRule,
AllowDeletions: newFalse(),
AllowForcePushes: &protectedBranch.AllowForcePush,
EnforceAdmins: newTrue(),
CheckRules: statusChecksRule,
},
}

Expand Down
6 changes: 3 additions & 3 deletions cron/internal/format/json_raw_results.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,13 @@ func addBranchProtectionRawResults(r *jsonScorecardRawResult, bp *checker.Branch
bp = &jsonBranchProtectionSettings{
AllowsDeletions: v.BranchProtectionRule.AllowDeletions,
AllowsForcePushes: v.BranchProtectionRule.AllowForcePushes,
RequiresCodeOwnerReviews: v.BranchProtectionRule.RequiredPullRequestReviews.RequireCodeOwnerReviews,
RequiresCodeOwnerReviews: v.BranchProtectionRule.PullRequestRule.RequireCodeOwnerReviews,
RequiresLinearHistory: v.BranchProtectionRule.RequireLinearHistory,
DismissesStaleReviews: v.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews,
DismissesStaleReviews: v.BranchProtectionRule.PullRequestRule.DismissStaleReviews,
EnforcesAdmins: v.BranchProtectionRule.EnforceAdmins,
RequiresStatusChecks: v.BranchProtectionRule.CheckRules.RequiresStatusChecks,
RequiresUpToDateBranchBeforeMerging: v.BranchProtectionRule.CheckRules.UpToDateBeforeMerge,
RequiredApprovingReviewCount: v.BranchProtectionRule.RequiredPullRequestReviews.RequiredApprovingReviewCount,
RequiredApprovingReviewCount: v.BranchProtectionRule.PullRequestRule.RequiredApprovingReviewCount,
StatusCheckContexts: v.BranchProtectionRule.CheckRules.Contexts,
}
}
Expand Down