Skip to content

Commit

Permalink
force push: re-apply refactor after huge changes on main
Browse files Browse the repository at this point in the history
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
  • Loading branch information
diogoteles08 committed Mar 21, 2024
1 parent c1066d9 commit fd0231b
Show file tree
Hide file tree
Showing 16 changed files with 111 additions and 111 deletions.
18 changes: 9 additions & 9 deletions checks/branch_protection_test.go
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
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
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
28 changes: 14 additions & 14 deletions clients/githubrepo/branches_test.go
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
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
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
Loading

0 comments on commit fd0231b

Please sign in to comment.