diff --git a/README.md b/README.md index 14a3210d..44198662 100644 --- a/README.md +++ b/README.md @@ -329,6 +329,18 @@ if: - "status-name-2" - "status-name-3" + # "has_successful_status" can be configured to count "skipped" statuses as + # successful. This can be useful in combination with path filters where + # workflows only run on parts of the tree. They are required to succeed only + # if they run. + # has_successful_status: + # options: + # skipped_is_success: true + # 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: diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 58782765..324fd075 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -48,6 +48,8 @@ type Options struct { RequestReview RequestReview `yaml:"request_review"` + CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"` + Methods *common.Methods `yaml:"methods"` } diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 19f88ed0..02500f01 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"}, + HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"deploy"}}, }, }, } @@ -688,7 +688,7 @@ func TestIsApproved(t *testing.T) { r := &Rule{ Requires: Requires{ Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}}, }, }, } @@ -701,7 +701,7 @@ func TestIsApproved(t *testing.T) { Requires: Requires{ Count: 1, Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}}, }, }, } @@ -717,7 +717,7 @@ func TestIsApproved(t *testing.T) { Organizations: []string{"everyone"}, }, Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}}, }, }, } @@ -825,7 +825,7 @@ func TestTrigger(t *testing.T) { r := &Rule{ Requires: Requires{ Conditions: predicate.Predicates{ - HasSuccessfulStatus: &predicate.HasSuccessfulStatus{"build"}, + HasSuccessfulStatus: &predicate.HasSuccessfulStatus{Statuses: []string{"build"}}, }, }, } diff --git a/policy/approval/parse_test.go b/policy/approval/parse_test.go index eb350942..879e909b 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,16 @@ func TestParsePolicy(t *testing.T) { enabled: true requires: admins: true +- name: rule8 + if: + has_successful_status: + - status1 +- name: rule9 + if: + has_successful_status: + statuses: [status2, status3] + options: + skipped_is_success: true ` var policy Policy @@ -119,6 +132,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/status.go b/policy/predicate/status.go index 6f1f61d3..242ebd84 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -23,30 +23,67 @@ import ( "github.com/pkg/errors" ) -type HasSuccessfulStatus []string +type hasSuccessfulStatusOptions struct { + SkippedIsSuccess bool `yaml:"skipped_is_success"` +} + +type HasSuccessfulStatus struct { + Options hasSuccessfulStatusOptions + Statuses []string `yaml:"statuses"` +} + +func NewHasSuccessfulStatus(statuses []string) *HasSuccessfulStatus { + return &HasSuccessfulStatus{ + Statuses: statuses, + } +} + +// UnmarshalYAML implements the yaml.Unmarshaler interface for HasSuccessfulStatus. +// This allows the predicate to be specified as either a list of strings or with options. +func (pred *HasSuccessfulStatus) UnmarshalYAML(unmarshal func(interface{}) error) error { + // Try to unmarshal as a list of strings first + statuses := []string{} + if err := unmarshal(&statuses); err == nil { + pred.Statuses = statuses + + return nil + } -var _ Predicate = HasSuccessfulStatus([]string{}) + // If that fails, try to unmarshal as the full structure + type rawHasSuccessfulStatus HasSuccessfulStatus + return unmarshal((*rawHasSuccessfulStatus)(pred)) +} + +var _ Predicate = HasSuccessfulStatus{} func (pred HasSuccessfulStatus) 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") + } + + allowedStatusConclusions := map[string]struct{}{ + "success": {}, + } predicateResult := common.PredicateResult{ ValuePhrase: "status checks", ConditionPhrase: "exist and pass", } - if err != nil { - return nil, errors.Wrap(err, "failed to list commit statuses") + if pred.Options.SkippedIsSuccess { + predicateResult.ConditionPhrase += " or are skipped" + allowedStatusConclusions["skipped"] = struct{}{} } 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 _, allowed := allowedStatusConclusions[result]; !allowed { failingStatuses = append(failingStatuses, status) } } @@ -65,7 +102,7 @@ func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context return &predicateResult, nil } - predicateResult.Values = pred + predicateResult.Values = pred.Statuses predicateResult.Satisfied = true return &predicateResult, nil } diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index 0ad200f5..d891a192 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -16,6 +16,7 @@ package predicate import ( "context" + "slices" "testing" "github.com/palantir/policy-bot/policy/common" @@ -24,10 +25,21 @@ import ( "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"}) + p := HasSuccessfulStatus{Statuses: []string{"status-name", "status-name-2"}} - runStatusTestCase(t, p, []StatusTestCase{ + commonTestCases := []StatusTestCase{ { "all statuses succeed", &pulltest.Context{ @@ -38,7 +50,6 @@ func TestHasSuccessfulStatus(t *testing.T) { }, &common.PredicateResult{ Satisfied: true, - Values: []string{"status-name", "status-name-2"}, }, }, { @@ -79,6 +90,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 +110,53 @@ 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"}, + }, + }, + } + + // Run tests with skipped statuses counting as failures + runStatusTestCase(t, p, commonTestCases) + runStatusTestCase(t, p, okOnlyIfSkippedAllowed) + + // Run tests with skipped statuses counting as successes + p.Options.SkippedIsSuccess = true + + for i := 0; i < len(commonTestCases); i++ { + commonTestCases[i].name += ", but skipped statuses are allowed" + } + for i := 0; i < len(okOnlyIfSkippedAllowed); i++ { + okOnlyIfSkippedAllowed[i].name += ", but skipped statuses are allowed" + okOnlyIfSkippedAllowed[i].ExpectedPredicateResult.Satisfied = true + } + runStatusTestCase(t, p, commonTestCases) + runStatusTestCase(t, p, okOnlyIfSkippedAllowed) } type StatusTestCase struct { @@ -100,6 +169,15 @@ func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) { ctx := context.Background() for _, tc := range cases { + // 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) + } + t.Run(tc.name, func(t *testing.T) { predicateResult, err := p.Evaluate(ctx, tc.context) if assert.NoError(t, err, "evaluation failed") {