diff --git a/checks/branch_protection_test.go b/checks/branch_protection_test.go index 34bf77d6283..929c20768c8 100644 --- a/checks/branch_protection_test.go +++ b/checks/branch_protection_test.go @@ -135,7 +135,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -159,10 +159,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLinearHistory: &falseVal, + RequireLastPushApproval: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -173,8 +174,8 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 8, - NumberOfInfo: 9, + NumberOfWarn: 9, + NumberOfInfo: 10, NumberOfDebug: 0, }, defaultBranch: main, @@ -193,10 +194,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { @@ -213,10 +215,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -228,7 +231,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 4, - NumberOfInfo: 14, + NumberOfInfo: 16, NumberOfDebug: 0, }, defaultBranch: main, @@ -247,10 +250,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { @@ -267,10 +271,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &trueVal, RequiredApprovingReviewCount: &oneVal, }, - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &trueVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -281,7 +286,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -302,10 +307,11 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { RequireCodeOwnerReviews: &falseVal, RequiredApprovingReviewCount: &zeroVal, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, { Name: &rel1, @@ -354,7 +360,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { Score: 0, NumberOfWarn: 4, NumberOfInfo: 0, - NumberOfDebug: 6, + NumberOfDebug: 8, }, nonadmin: true, defaultBranch: main, diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index fd50d4c4663..7703cab920c 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -373,18 +373,31 @@ func adminReviewProtection(branch *clients.BranchRef, dl checker.DetailLogger) ( // Only log information if the branch is protected. log := branch.Protected != nil && *branch.Protected - if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge != nil { + // Process UpToDateBeforeMerge value. + if branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge == nil { + debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) + } else { // Note: `This setting will not take effect unless at least one status check is enabled`. max++ - switch *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { - case true: + if *branch.BranchProtectionRule.CheckRules.UpToDateBeforeMerge { info(dl, log, "status checks require up-to-date branches for '%s'", *branch.Name) score++ - default: + } else { warn(dl, log, "status checks do not require up-to-date branches for '%s'", *branch.Name) } + } + + // Process RequireLastPushApproval value. + if branch.BranchProtectionRule.RequireLastPushApproval == nil { + debug(dl, log, "unable to retrieve whether 'last push approval' is required to merge on branch '%s'", *branch.Name) } else { - debug(dl, log, "unable to retrieve whether up-to-date branches are needed to merge on branch '%s'", *branch.Name) + max++ + if *branch.BranchProtectionRule.RequireLastPushApproval { + info(dl, log, "'last push approval' enabled on branch '%s'", *branch.Name) + score++ + } else { + warn(dl, log, "'last push approval' disabled on branch '%s'", *branch.Name) + } } return score, max @@ -401,10 +414,10 @@ func adminThoroughReviewProtection(branch *clients.BranchRef, dl checker.DetailL max++ switch *branch.BranchProtectionRule.RequiredPullRequestReviews.DismissStaleReviews { case true: - info(dl, log, "Stale review dismissal enabled on branch '%s'", *branch.Name) + info(dl, log, "stale review dismissal enabled on branch '%s'", *branch.Name) score++ case false: - warn(dl, log, "Stale review dismissal disabled on branch '%s'", *branch.Name) + warn(dl, log, "stale review dismissal disabled on branch '%s'", *branch.Name) } } else { debug(dl, log, "unable to retrieve review dismissal on branch '%s'", *branch.Name) diff --git a/checks/evaluation/branch_protection_test.go b/checks/evaluation/branch_protection_test.go index 4f7b3b18f86..5fd5e541edb 100644 --- a/checks/evaluation/branch_protection_test.go +++ b/checks/evaluation/branch_protection_test.go @@ -54,7 +54,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -62,10 +62,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - AllowDeletions: &falseVal, - AllowForcePushes: &falseVal, - RequireLinearHistory: &falseVal, - EnforceAdmins: &falseVal, + AllowDeletions: &falseVal, + AllowForcePushes: &falseVal, + RequireLinearHistory: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, @@ -86,7 +87,7 @@ func TestIsBranchProtected(t *testing.T) { Score: 0, NumberOfWarn: 2, NumberOfInfo: 0, - NumberOfDebug: 3, + NumberOfDebug: 4, }, branch: &clients.BranchRef{ Name: &branchVal, @@ -98,7 +99,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 4, + NumberOfWarn: 5, NumberOfInfo: 4, NumberOfDebug: 0, }, @@ -116,10 +117,11 @@ func TestIsBranchProtected(t *testing.T) { UpToDateBeforeMerge: &trueVal, Contexts: []string{"foo"}, }, - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, }, }, }, @@ -128,7 +130,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -136,10 +138,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, RequiredPullRequestReviews: clients.PullRequestReviewRule{ DismissStaleReviews: &falseVal, RequireCodeOwnerReviews: &falseVal, @@ -158,7 +161,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -166,10 +169,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, UpToDateBeforeMerge: &falseVal, @@ -188,7 +192,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 3, - NumberOfWarn: 4, + NumberOfWarn: 5, NumberOfInfo: 4, NumberOfDebug: 0, }, @@ -196,10 +200,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -218,7 +223,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 2, - NumberOfWarn: 5, + NumberOfWarn: 6, NumberOfInfo: 3, NumberOfDebug: 0, }, @@ -226,10 +231,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -248,7 +254,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 1, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -256,10 +262,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &trueVal, - AllowDeletions: &falseVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &trueVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, @@ -279,7 +286,7 @@ func TestIsBranchProtected(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 1, - NumberOfWarn: 6, + NumberOfWarn: 7, NumberOfInfo: 2, NumberOfDebug: 0, }, @@ -287,10 +294,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &falseVal, - RequireLinearHistory: &falseVal, - AllowForcePushes: &falseVal, - AllowDeletions: &trueVal, + EnforceAdmins: &falseVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &falseVal, + AllowForcePushes: &falseVal, + AllowDeletions: &trueVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &falseVal, @@ -310,17 +318,18 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 2, - NumberOfInfo: 7, + NumberOfInfo: 8, NumberOfDebug: 0, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + RequireLastPushApproval: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &trueVal, @@ -340,17 +349,18 @@ func TestIsBranchProtected(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfInfo: 8, NumberOfDebug: 0, }, branch: &clients.BranchRef{ Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLinearHistory: &trueVal, + RequireLastPushApproval: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &trueVal, UpToDateBeforeMerge: &trueVal, @@ -369,8 +379,8 @@ func TestIsBranchProtected(t *testing.T) { name: "Branches are protected and require codeowner review, but file is not present", expected: scut.TestReturn{ Error: nil, - Score: 8, - NumberOfWarn: 2, + Score: 5, + NumberOfWarn: 3, NumberOfInfo: 7, NumberOfDebug: 0, }, @@ -378,10 +388,11 @@ func TestIsBranchProtected(t *testing.T) { Name: &branchVal, Protected: &trueVal, BranchProtectionRule: clients.BranchProtectionRule{ - EnforceAdmins: &trueVal, - RequireLinearHistory: &trueVal, - AllowForcePushes: &falseVal, - AllowDeletions: &falseVal, + EnforceAdmins: &trueVal, + RequireLastPushApproval: &falseVal, + RequireLinearHistory: &trueVal, + AllowForcePushes: &falseVal, + AllowDeletions: &falseVal, CheckRules: clients.StatusChecksRule{ RequiresStatusChecks: &falseVal, UpToDateBeforeMerge: &trueVal, diff --git a/clients/branch.go b/clients/branch.go index 0481a893fdd..ff11e4a683e 100644 --- a/clients/branch.go +++ b/clients/branch.go @@ -28,6 +28,7 @@ type BranchProtectionRule struct { AllowForcePushes *bool RequireLinearHistory *bool EnforceAdmins *bool + RequireLastPushApproval *bool CheckRules StatusChecksRule } diff --git a/clients/githubrepo/branches.go b/clients/githubrepo/branches.go index 8250929ecab..027ca56be19 100644 --- a/clients/githubrepo/branches.go +++ b/clients/githubrepo/branches.go @@ -36,19 +36,20 @@ const ( query { repository(owner: "laurentsimon", name: "test3") { branchProtectionRules(first: 100) { - allowsDeletions - allowsForcePushes - dismissesStaleReviews - isAdminEnforced - ... - pattern - matchingRefs(first: 100) { - nodes { - name - - } - } - } + edges{ + node{ + allowsDeletions + allowsForcePushes + dismissesStaleReviews + isAdminEnforced + ... + pattern + matchingRefs(first: 100) { + nodes { + name + } + } + } } refs(first: 100, refPrefix: "refs/heads/") { nodes { @@ -86,6 +87,7 @@ type branchProtectionRule struct { RequiredApprovingReviewCount *int32 RequiresCodeOwnerReviews *bool RequiresLinearHistory *bool + RequireLastPushApproval *bool RequiredStatusCheckContexts []string // TODO: verify there is no conflicts. // BranchProtectionRuleConflicts interface{} @@ -184,6 +186,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) diff --git a/docs/checks.md b/docs/checks.md index 76ba275b10b..5d39e313998 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -105,7 +105,8 @@ Tier 1 Requirements (3/10 points): - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - - Required reviewers >=1 ​ + - Required reviewers >=1 + - For administrators: Last push review - For administrators: Strict status checks (require branches to be up-to-date before merging) Tier 3 Requirements (8/10 points): diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 63c0f97ee34..d849772d351 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -194,7 +194,8 @@ checks: - For administrators: Include administrator for review Tier 2 Requirements (6/10 points): - - Required reviewers >=1 ​ + - Required reviewers >=1 + - For administrators: Last push review - For administrators: Strict status checks (require branches to be up-to-date before merging) Tier 3 Requirements (8/10 points): diff --git a/e2e/branch_protection_test.go b/e2e/branch_protection_test.go index b1ef2020dfb..7a386273346 100644 --- a/e2e/branch_protection_test.go +++ b/e2e/branch_protection_test.go @@ -49,7 +49,7 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Score: 6, NumberOfWarn: 2, NumberOfInfo: 4, - NumberOfDebug: 3, + NumberOfDebug: 4, } result := checks.BranchProtection(&req) // UPGRADEv2: to remove. @@ -84,10 +84,10 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { result := checks.BranchProtection(&req) // New version. - Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue()) + Expect(scut.ValidateTestReturn(nil, "branch protection accessible none", &expected, &result, &dl)).Should(BeTrue()) Expect(repoClient.Close()).Should(BeNil()) }) - It("Should fail to return branch protection on other repositories", func() { + It("Should fail to return branch protection on other repositories patch", func() { skipIfTokenIsNot(patTokenType, "PAT only") dl := scut.TestDetailLogger{} @@ -107,12 +107,12 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() { Score: 1, NumberOfWarn: 3, NumberOfInfo: 3, - NumberOfDebug: 3, + NumberOfDebug: 4, } result := checks.BranchProtection(&req) // New version. - Expect(scut.ValidateTestReturn(nil, "branch protection accessible", &expected, &result, &dl)).Should(BeTrue()) + Expect(scut.ValidateTestReturn(nil, "branch protection accessible patch", &expected, &result, &dl)).Should(BeTrue()) Expect(repoClient.Close()).Should(BeNil()) }) })