diff --git a/README.md b/README.md index 14a3210d..c350e8eb 100644 --- a/README.md +++ b/README.md @@ -322,6 +322,7 @@ if: deletions: "> 100" total: "> 200" + # DEPRECATED: Use "has_status" below instead, which is more flexible. # "has_successful_status" is satisfied if the status checks that are specified # are marked successful on the head commit of the pull request. has_successful_status: @@ -329,6 +330,16 @@ if: - "status-name-2" - "status-name-3" + # "has_status" is satisfied if the status checks that are specified are + # finished and concluded with one of the conclusions specified. + # "conclusions" is optional and defaults to ["success"]. + has_status: + conclusions: ["success", "skipped"] + statuses: + - "status-name-1" + - "status-name-2" + - "status-name-3" + # "has_labels" is satisfied if the pull request has the specified labels # applied has_labels: @@ -510,16 +521,17 @@ requires: # count as approved. If present, conditions are an additional requirement # beyond the approvals required by "count". # - # For example, if "count" is 1 and "conditions" contains the "has_successful_status" + # For example, if "count" is 1 and "conditions" contains the "has_status" # condition with the "build" status, the rule is only approved once the # "build" status check passes and one authorized reviewer leaves a review. conditions: - # The "conditions" block accepts all of the keys documented as part - # of the "if" block for predicates. The "has_successful_status" key is - # shown here as an example of one type of condition. - has_successful_status: - - "build" - - "vulnerability scan" + # The "conditions" block accepts all of the keys documented as part of the + # "if" block for predicates. The "has_status" key is shown here as an + # example of one type of condition. + has_status: + statuses: + - "build" + - "vulnerability scan" ``` ### Approval Policies diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 19f88ed0..8fa42b46 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -676,7 +676,7 @@ func TestIsApproved(t *testing.T) { r := &Rule{ Requires: Requires{ Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"deploy"}, + HasStatus: &predicate.HasStatus{Statuses: []string{"deploy"}}, }, }, } @@ -688,7 +688,7 @@ func TestIsApproved(t *testing.T) { r := &Rule{ Requires: Requires{ Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasStatus: &predicate.HasStatus{Statuses: []string{"build"}}, }, }, } @@ -701,7 +701,7 @@ func TestIsApproved(t *testing.T) { Requires: Requires{ Count: 1, Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasStatus: &predicate.HasStatus{Statuses: []string{"build"}}, }, }, } @@ -717,7 +717,7 @@ func TestIsApproved(t *testing.T) { Organizations: []string{"everyone"}, }, Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasStatus: &predicate.HasStatus{Statuses: []string{"build"}}, }, }, } @@ -825,7 +825,7 @@ func TestTrigger(t *testing.T) { r := &Rule{ Requires: Requires{ Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasStatus: predicate.NewHasStatus([]string{"status1"}, []string{"skipped", "success"}), }, }, } diff --git a/policy/approval/parse_test.go b/policy/approval/parse_test.go index eb350942..9dca0425 100644 --- a/policy/approval/parse_test.go +++ b/policy/approval/parse_test.go @@ -35,6 +35,9 @@ func TestParsePolicy(t *testing.T) { - and: - rule6 - rule7 + - or: + - rule8 + - rule9 ` ruleText := ` @@ -67,6 +70,15 @@ func TestParsePolicy(t *testing.T) { enabled: true requires: admins: true +- name: rule8 + if: + has_successful_status: + - status1 +- name: rule9 + if: + has_status: + conclusions: ["success", "skipped"] + statuses: [status2, status3] ` var policy Policy @@ -119,6 +131,16 @@ func TestParsePolicy(t *testing.T) { &RuleRequirement{ rule: rules[6], }, + &OrRequirement{ + requirements: []common.Evaluator{ + &RuleRequirement{ + rule: rules[7], + }, + &RuleRequirement{ + rule: rules[8], + }, + }, + }, }, }, }, diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go index d62fa9b1..eeb9bb53 100644 --- a/policy/predicate/predicates.go +++ b/policy/predicate/predicates.go @@ -29,6 +29,10 @@ type Predicates struct { ModifiedLines *ModifiedLines `yaml:"modified_lines"` + HasStatus *HasStatus `yaml:"has_status"` + // `has_successful_status` is a deprecated field that is kept for backwards + // compatibility. `has_status` replaces it, and can accept any conclusion + // rather than just "success". HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"` HasLabels *HasLabels `yaml:"has_labels"` @@ -78,6 +82,10 @@ func (p *Predicates) Predicates() []Predicate { ps = append(ps, Predicate(p.ModifiedLines)) } + if p.HasStatus != nil { + ps = append(ps, Predicate(p.HasStatus)) + } + if p.HasSuccessfulStatus != nil { ps = append(ps, Predicate(p.HasSuccessfulStatus)) } diff --git a/policy/predicate/status.go b/policy/predicate/status.go index 6f1f61d3..9727d9cf 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -16,6 +16,8 @@ package predicate import ( "context" + "fmt" + "slices" "strings" "github.com/palantir/policy-bot/policy/common" @@ -23,30 +25,46 @@ import ( "github.com/pkg/errors" ) -type HasSuccessfulStatus []string - -var _ Predicate = HasSuccessfulStatus([]string{}) +type AllowedConclusions []string -func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { - statuses, err := prctx.LatestStatuses() +type HasStatus struct { + Conclusions AllowedConclusions `yaml:"conclusions"` + Statuses []string `yaml:"statuses"` +} - predicateResult := common.PredicateResult{ - ValuePhrase: "status checks", - ConditionPhrase: "exist and pass", +func NewHasStatus(statuses []string, conclusions []string) *HasStatus { + return &HasStatus{ + Conclusions: conclusions, + Statuses: statuses, } +} +var _ Predicate = HasStatus{} + +func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { + statuses, err := prctx.LatestStatuses() if err != nil { return nil, errors.Wrap(err, "failed to list commit statuses") } + conclusions := pred.Conclusions + if len(conclusions) == 0 { + conclusions = AllowedConclusions{"success"} + } + + predicateResult := common.PredicateResult{ + ValuePhrase: "status checks", + ConditionPhrase: fmt.Sprintf("exist and have conclusion %s", conclusions.joinWithOr()), + } + var missingResults []string var failingStatuses []string - for _, status := range pred { + for _, status := range pred.Statuses { result, ok := statuses[status] if !ok { missingResults = append(missingResults, status) } - if result != "success" { + if !slices.Contains(conclusions, result) { failingStatuses = append(failingStatuses, status) } } @@ -60,16 +78,58 @@ func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context if len(failingStatuses) > 0 { predicateResult.Values = failingStatuses - predicateResult.Description = "One or more statuses has not passed: " + strings.Join(failingStatuses, ",") + predicateResult.Description = fmt.Sprintf("One or more statuses has not concluded with %s: %s", pred.Conclusions.joinWithOr(), strings.Join(failingStatuses, ",")) predicateResult.Satisfied = false return &predicateResult, nil } - predicateResult.Values = pred + predicateResult.Values = pred.Statuses predicateResult.Satisfied = true return &predicateResult, nil } +func (pred HasStatus) Trigger() common.Trigger { + return common.TriggerStatus +} + +// HasSuccessfulStatus checks that the specified statuses have a successful +// conclusion. +// +// Deprecated: use the more flexible `HasStatus` with `conclusions: ["success"]` +// instead. +type HasSuccessfulStatus []string + +var _ Predicate = HasSuccessfulStatus{} + +func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { + return HasStatus{ + Statuses: pred, + }.Evaluate(ctx, prctx) +} + func (pred HasSuccessfulStatus) Trigger() common.Trigger { return common.TriggerStatus } + +// joinWithOr returns a string that represents the allowed conclusions in a +// format that can be used in a sentence. For example, if the allowed +// conclusions are "success" and "failure", this will return "success or +// failure". If there are more than two conclusions, the first n-1 will be +// separated by commas. +func (c AllowedConclusions) joinWithOr() string { + slices.Sort(c) + + length := len(c) + switch length { + case 0: + return "" + case 1: + return c[0] + case 2: + return c[0] + " or " + c[1] + } + + head, tail := c[:length-1], c[length-1] + + return strings.Join(head, ", ") + ", or " + tail +} diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index 0ad200f5..952fc1b8 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -16,18 +16,38 @@ package predicate import ( "context" + "fmt" + "slices" + "strings" "testing" + "github.com/google/go-github/v62/github" "github.com/palantir/policy-bot/policy/common" "github.com/palantir/policy-bot/pull" "github.com/palantir/policy-bot/pull/pulltest" "github.com/stretchr/testify/assert" ) +func keysSorted[V any](m map[string]V) []string { + r := make([]string, 0, len(m)) + + for k := range m { + r = append(r, k) + } + + slices.Sort(r) + return r +} + func TestHasSuccessfulStatus(t *testing.T) { - p := HasSuccessfulStatus([]string{"status-name", "status-name-2"}) + hasStatus := HasStatus{Statuses: []string{"status-name", "status-name-2"}} + hasStatusSkippedOk := HasStatus{ + Statuses: []string{"status-name", "status-name-2"}, + Conclusions: AllowedConclusions{"success", "skipped"}, + } + hasSuccessfulStatus := HasSuccessfulStatus{"status-name", "status-name-2"} - runStatusTestCase(t, p, []StatusTestCase{ + commonTestCases := []StatusTestCase{ { "all statuses succeed", &pulltest.Context{ @@ -38,7 +58,6 @@ func TestHasSuccessfulStatus(t *testing.T) { }, &common.PredicateResult{ Satisfied: true, - Values: []string{"status-name", "status-name-2"}, }, }, { @@ -79,6 +98,18 @@ func TestHasSuccessfulStatus(t *testing.T) { Values: []string{"status-name-2"}, }, }, + { + "a status does not exist, the other status is skipped", + &pulltest.Context{ + LatestStatusesValue: map[string]string{ + "status-name-2": "skipped", + }, + }, + &common.PredicateResult{ + Satisfied: false, + Values: []string{"status-name"}, + }, + }, { "multiple statuses do not exist", &pulltest.Context{}, @@ -87,7 +118,65 @@ func TestHasSuccessfulStatus(t *testing.T) { Values: []string{"status-name", "status-name-2"}, }, }, - }) + } + + okOnlyIfSkippedAllowed := []StatusTestCase{ + { + "a status is skipped", + &pulltest.Context{ + LatestStatusesValue: map[string]string{ + "status-name": "success", + "status-name-2": "skipped", + }, + }, + &common.PredicateResult{ + Satisfied: false, + Values: []string{"status-name-2"}, + }, + }, + { + "all statuses are skipped", + &pulltest.Context{ + LatestStatusesValue: map[string]string{ + "status-name": "skipped", + "status-name-2": "skipped", + }, + }, + &common.PredicateResult{ + Satisfied: false, + Values: []string{"status-name", "status-name-2"}, + }, + }, + } + + testSuites := []StatusTestSuite{ + {predicate: hasStatus, testCases: commonTestCases}, + {predicate: hasStatus, testCases: okOnlyIfSkippedAllowed}, + {predicate: hasSuccessfulStatus, testCases: commonTestCases}, + {predicate: hasSuccessfulStatus, testCases: okOnlyIfSkippedAllowed}, + { + nameSuffix: "skipped allowed", + predicate: hasStatusSkippedOk, + testCases: commonTestCases, + }, + { + nameSuffix: "skipped allowed", + predicate: hasStatusSkippedOk, + testCases: okOnlyIfSkippedAllowed, + overrideSatisfied: github.Bool(true), + }, + } + + for _, suite := range testSuites { + runStatusTestCase(t, suite.predicate, suite) + } +} + +type StatusTestSuite struct { + nameSuffix string + predicate Predicate + testCases []StatusTestCase + overrideSatisfied *bool } type StatusTestCase struct { @@ -96,11 +185,32 @@ type StatusTestCase struct { ExpectedPredicateResult *common.PredicateResult } -func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) { +func runStatusTestCase(t *testing.T, p Predicate, suite StatusTestSuite) { ctx := context.Background() - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range suite.testCases { + if suite.overrideSatisfied != nil { + tc.ExpectedPredicateResult.Satisfied = *suite.overrideSatisfied + } + + // If the test case expects the predicate to be satisfied, we always + // expect the values to contain all the statuses. Doing this here lets + // us use the same testcases when we allow and don't allow skipped + // statuses. + if tc.ExpectedPredicateResult.Satisfied { + statuses, _ := tc.context.LatestStatuses() + tc.ExpectedPredicateResult.Values = keysSorted(statuses) + } + + // `predicate.HasStatus` -> `HasStatus` + _, predicateType, _ := strings.Cut(fmt.Sprintf("%T", p), ".") + testName := fmt.Sprintf("%s_%s", predicateType, tc.name) + + if suite.nameSuffix != "" { + testName += "_" + suite.nameSuffix + } + + t.Run(testName, func(t *testing.T) { predicateResult, err := p.Evaluate(ctx, tc.context) if assert.NoError(t, err, "evaluation failed") { assertPredicateResult(t, tc.ExpectedPredicateResult, predicateResult) @@ -108,3 +218,43 @@ func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) { }) } } + +func TestJoinWithOr(t *testing.T) { + testCases := []struct { + name string + input AllowedConclusions + expected string + }{ + { + "empty", + AllowedConclusions{}, + "", + }, + { + "single", + AllowedConclusions{"a"}, + "a", + }, + { + "two", + AllowedConclusions{"a", "b"}, + "a or b", + }, + { + "three", + AllowedConclusions{"a", "b", "c"}, + "a, b, or c", + }, + { + "conclusions get sorted", + AllowedConclusions{"c", "a", "b"}, + "a, b, or c", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, tc.input.joinWithOr()) + }) + } +}