From 7aaacbfe2759d9c03545dcbd47a829b1285fe705 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Thu, 27 Jun 2024 20:15:05 +0100 Subject: [PATCH] Add support for allowing skipped checks in `has_successful_status` The `has_successful_status` predicate currently requires predicates to be present and successful in order to pass. But workflows can be run conditionally - for example only if certain paths change - and it is currently not very convenient to write a policy which considers such skipped workflows as passing. The only feasible workaround is to duplicate the path filters in the policy, and this quickly gets unwieldy and prone to getting out-of-sync in large repositories. Here we add direct support for specifying such rules. This is done by introducing a new alernative form that `has_successful_status` predicates can take: ```yaml has_successful_status: options: skipped_is_success: true statuses: - "status 1" - "status 2" ``` In this mode, we will consider the `skipped` result as acceptable. The current form: ```yaml has_successful_status: - "status 1" - "status 2" ``` remains supported. We have done this by implementing a custom unmarshaling function to be able to handle both forms. Closes: #760 --- README.md | 12 +++++ policy/approval/approve.go | 2 + policy/approval/approve_test.go | 10 ++-- policy/approval/parse_test.go | 23 +++++++++ policy/predicate/status.go | 51 ++++++++++++++++--- policy/predicate/status_test.go | 86 +++++++++++++++++++++++++++++++-- 6 files changed, 168 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 14a3210d8..441986629 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 58782765a..324fd075f 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 19f88ed07..02500f018 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 eb350942c..879e909b9 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 6f1f61d3e..242ebd843 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 0ad200f5b..d891a1921 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") {