From b911ad5f05f6956ca70666e2130fff78c1b8df7f Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 1 Jul 2024 13:28:27 +0100 Subject: [PATCH] Rework to expose `has_status` Implement `has_successful_status` in terms of `has_status` and deprecate it. This avoids us having two forms for one predidcate. An example would be ```yaml has_status: conclusions: ["success", "skipped"] statuses: - "status 1" - "status 2" ``` --- README.md | 42 +++++-------- policy/approval/approve_test.go | 10 +-- policy/approval/parse_test.go | 5 +- policy/predicate/predicates.go | 10 ++- policy/predicate/status.go | 106 ++++++++++++++++++++++++-------- policy/predicate/status_test.go | 4 +- 6 files changed, 114 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 441986629..b19f3db95 100644 --- a/README.md +++ b/README.md @@ -322,24 +322,15 @@ if: deletions: "> 100" total: "> 200" - # "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: - - "status-name-1" - - "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_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 @@ -522,16 +513,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 02500f018..7dac0ac10 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{Statuses: []string{"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{Statuses: []string{"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{Statuses: []string{"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{Statuses: []string{"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{Statuses: []string{"build"}}, + HasStatus: &predicate.HasStatus{Statuses: []string{"build"}}, }, }, } diff --git a/policy/approval/parse_test.go b/policy/approval/parse_test.go index 879e909b9..9dca04250 100644 --- a/policy/approval/parse_test.go +++ b/policy/approval/parse_test.go @@ -76,10 +76,9 @@ func TestParsePolicy(t *testing.T) { - status1 - name: rule9 if: - has_successful_status: + has_status: + conclusions: ["success", "skipped"] statuses: [status2, status3] - options: - skipped_is_success: true ` var policy Policy diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go index d62fa9b13..366911f2b 100644 --- a/policy/predicate/predicates.go +++ b/policy/predicate/predicates.go @@ -29,7 +29,11 @@ type Predicates struct { ModifiedLines *ModifiedLines `yaml:"modified_lines"` - HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"` + 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 *HasStatus `yaml:"has_successful_status"` HasLabels *HasLabels `yaml:"has_labels"` @@ -78,8 +82,8 @@ func (p *Predicates) Predicates() []Predicate { ps = append(ps, Predicate(p.ModifiedLines)) } - if p.HasSuccessfulStatus != nil { - ps = append(ps, Predicate(p.HasSuccessfulStatus)) + if p.HasStatus != nil { + ps = append(ps, Predicate(p.HasStatus)) } if p.HasLabels != nil { diff --git a/policy/predicate/status.go b/policy/predicate/status.go index 242ebd843..53ccf334d 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -16,6 +16,7 @@ package predicate import ( "context" + "fmt" "strings" "github.com/palantir/policy-bot/policy/common" @@ -23,24 +24,83 @@ import ( "github.com/pkg/errors" ) -type hasSuccessfulStatusOptions struct { - SkippedIsSuccess bool `yaml:"skipped_is_success"` +type unit struct{} +type set map[string]unit +type allowedConclusions set + +// UnmarshalYAML implements the yaml.Unmarshaler interface for allowedConclusions. +// This allows the predicate to be specified in the input as a list of strings, +// which we then convert to a set of strings, for easy membership testing. +func (c *allowedConclusions) UnmarshalYAML(unmarshal func(interface{}) error) error { + var conclusions []string + if err := unmarshal(&conclusions); err != nil { + return fmt.Errorf("failed to unmarshal conclusions: %v", err) + } + + *c = make(allowedConclusions, len(conclusions)) + for _, conclusion := range conclusions { + (*c)[conclusion] = unit{} + } + + return nil +} + +// 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 { + length := len(c) + + keys := make([]string, 0, length) + for key := range c { + keys = append(keys, key) + } + + switch length { + case 0: + return "" + case 1: + return keys[0] + case 2: + return keys[0] + " or " + keys[1] + } + + head, tail := keys[:length-1], keys[length-1] + + return strings.Join(head, ", ") + ", or " + tail +} + +// If unspecified, require the status to be successful. +var defaultConclusions allowedConclusions = allowedConclusions{ + "success": unit{}, } -type HasSuccessfulStatus struct { - Options hasSuccessfulStatusOptions - Statuses []string `yaml:"statuses"` +type HasStatus struct { + // Can be one of: action_required, cancelled, failure, neutral, success, skipped, stale, timed_out + Conclusions allowedConclusions `yaml:"conclusions"` + Statuses []string `yaml:"statuses"` } -func NewHasSuccessfulStatus(statuses []string) *HasSuccessfulStatus { - return &HasSuccessfulStatus{ - Statuses: statuses, +func NewHasStatus(statuses []string, conclusions []string) *HasStatus { + conclusionsSet := make(allowedConclusions, len(conclusions)) + for _, conclusion := range conclusions { + conclusionsSet[conclusion] = unit{} + } + return &HasStatus{ + Conclusions: conclusionsSet, + 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 { +// UnmarshalYAML implements the yaml.Unmarshaler interface for HasStatus. +// This supports unmarshalling the predicate in two forms: +// 1. A list of strings, which are the statuses to check for. This is the +// deprecated `has_successful_status` format. +// 2. A full structure with `statuses` and `conclusions` fields as in +// `has_status`. +func (pred *HasStatus) UnmarshalYAML(unmarshal func(interface{}) error) error { // Try to unmarshal as a list of strings first statuses := []string{} if err := unmarshal(&statuses); err == nil { @@ -50,30 +110,26 @@ func (pred *HasSuccessfulStatus) UnmarshalYAML(unmarshal func(interface{}) error } // If that fails, try to unmarshal as the full structure - type rawHasSuccessfulStatus HasSuccessfulStatus + type rawHasSuccessfulStatus HasStatus return unmarshal((*rawHasSuccessfulStatus)(pred)) } -var _ Predicate = HasSuccessfulStatus{} +var _ Predicate = HasStatus{} -func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { +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") } - allowedStatusConclusions := map[string]struct{}{ - "success": {}, - } - predicateResult := common.PredicateResult{ ValuePhrase: "status checks", - ConditionPhrase: "exist and pass", + ConditionPhrase: fmt.Sprintf("exist and have conclusion %s", pred.Conclusions.joinWithOr()), } - if pred.Options.SkippedIsSuccess { - predicateResult.ConditionPhrase += " or are skipped" - allowedStatusConclusions["skipped"] = struct{}{} + conclusions := pred.Conclusions + if len(conclusions) == 0 { + conclusions = defaultConclusions } var missingResults []string @@ -83,7 +139,7 @@ func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context if !ok { missingResults = append(missingResults, status) } - if _, allowed := allowedStatusConclusions[result]; !allowed { + if _, allowed := conclusions[result]; !allowed { failingStatuses = append(failingStatuses, status) } } @@ -97,7 +153,7 @@ 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 } @@ -107,6 +163,6 @@ func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context return &predicateResult, nil } -func (pred HasSuccessfulStatus) Trigger() common.Trigger { +func (pred HasStatus) Trigger() common.Trigger { return common.TriggerStatus } diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index d891a1921..7a4cf9981 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -37,7 +37,7 @@ func keysSorted[V any](m map[string]V) []string { } func TestHasSuccessfulStatus(t *testing.T) { - p := HasSuccessfulStatus{Statuses: []string{"status-name", "status-name-2"}} + p := HasStatus{Statuses: []string{"status-name", "status-name-2"}} commonTestCases := []StatusTestCase{ { @@ -146,7 +146,7 @@ func TestHasSuccessfulStatus(t *testing.T) { runStatusTestCase(t, p, okOnlyIfSkippedAllowed) // Run tests with skipped statuses counting as successes - p.Options.SkippedIsSuccess = true + p.Conclusions = allowedConclusions{"success": {}, "skipped": {}} for i := 0; i < len(commonTestCases); i++ { commonTestCases[i].name += ", but skipped statuses are allowed"