From caba90e0ab15f7b7138d240b8b493bf5d5f3444c Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Sun, 7 Jul 2024 11:49:23 +0100 Subject: [PATCH 1/8] Add utility functions to allow predicates to take lists of conclusions If a predicate has an `allowedConclusions` member, it will unmarshal from a list `["a", "b", "c"]` into a set, so that the handler can easily check if the incoming conclusion was allowed. --- policy/predicate/predicate.go | 61 +++++++++++++++++++++ policy/predicate/predicate_test.go | 87 ++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/policy/predicate/predicate.go b/policy/predicate/predicate.go index 15a244a4..cb705e11 100644 --- a/policy/predicate/predicate.go +++ b/policy/predicate/predicate.go @@ -16,6 +16,9 @@ package predicate import ( "context" + "fmt" + "slices" + "strings" "github.com/palantir/policy-bot/policy/common" "github.com/palantir/policy-bot/pull" @@ -27,3 +30,61 @@ type Predicate interface { // Evaluate determines if the predicate is satisfied. Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) } + +type unit struct{} +type set map[string]unit + +// allowedConclusions can be one of: +// action_required, cancelled, failure, neutral, success, skipped, stale, +// timed_out +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) + } + slices.Sort(keys) + + 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{}, +} diff --git a/policy/predicate/predicate_test.go b/policy/predicate/predicate_test.go index b9fff863..a27a4653 100644 --- a/policy/predicate/predicate_test.go +++ b/policy/predicate/predicate_test.go @@ -19,6 +19,7 @@ import ( "github.com/palantir/policy-bot/policy/common" "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v2" ) func assertPredicateResult(t *testing.T, expected, actual *common.PredicateResult) { @@ -27,3 +28,89 @@ func assertPredicateResult(t *testing.T, expected, actual *common.PredicateResul assert.Equal(t, expected.ConditionsMap, actual.ConditionsMap, "conditions were not correct") assert.Equal(t, expected.ConditionValues, actual.ConditionValues, "conditions were not correct") } + +func TestUnmarshalAllowedConclusions(t *testing.T) { + testCases := []struct { + name string + input string + expected allowedConclusions + expectedErr bool + }{ + { + name: "empty", + input: "", + expected: nil, + }, + { + name: "single", + input: `["success"]`, + expected: allowedConclusions{"success": unit{}}, + }, + { + name: "multiple", + input: `["success", "failure"]`, + expected: allowedConclusions{"success": unit{}, "failure": unit{}}, + }, + { + name: "repeat", + input: `["success", "success"]`, + expected: allowedConclusions{"success": unit{}}, + }, + { + name: "invalid", + input: `notyaml`, + expectedErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var actual allowedConclusions + err := yaml.UnmarshalStrict([]byte(tc.input), &actual) + + if tc.expectedErr { + assert.Error(t, err, "UnmarshalStrict should have failed") + return + } + + assert.NoError(t, err, "UnmarshalStrict failed") + assert.Equal(t, tc.expected, actual, "values were not correct") + }) + } +} + +func TestJoinWithOr(t *testing.T) { + testCases := []struct { + name string + input allowedConclusions + expected string + }{ + { + name: "empty", + input: nil, + expected: "", + }, + { + name: "one conclusion", + input: allowedConclusions{"success": unit{}}, + expected: "success", + }, + { + name: "two conclusions", + input: allowedConclusions{"success": unit{}, "failure": unit{}}, + expected: "failure or success", + }, + { + name: "three conclusions", + input: allowedConclusions{"success": unit{}, "failure": unit{}, "cancelled": unit{}}, + expected: "cancelled, failure, or success", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual := tc.input.joinWithOr() + assert.Equal(t, tc.expected, actual, "values were not correct") + }) + } +} From a5acfbc7fb6c19fc57a959e3f46efafb96fe6915 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Thu, 27 Jun 2024 20:15:05 +0100 Subject: [PATCH 2/8] 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 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") { From d96d2a0ad934536344d8ee84f942bb3f39afe8ed Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 1 Jul 2024 13:28:27 +0100 Subject: [PATCH 3/8] 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 | 56 +++++++++++++++++---------------- policy/predicate/status_test.go | 4 +-- 6 files changed, 64 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 44198662..b19f3db9 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 02500f01..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{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.NewHasStatus([]string{"status1"}, []string{"skipped", "success"}), }, }, } diff --git a/policy/approval/parse_test.go b/policy/approval/parse_test.go index 879e909b..9dca0425 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 d62fa9b1..459cf7b9 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,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 242ebd84..d15131d8 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,29 @@ import ( "github.com/pkg/errors" ) -type hasSuccessfulStatusOptions struct { - SkippedIsSuccess bool `yaml:"skipped_is_success"` +type HasStatus struct { + Conclusions allowedConclusions `yaml:"conclusions"` + Statuses []string `yaml:"statuses"` } -type HasSuccessfulStatus struct { - Options hasSuccessfulStatusOptions - 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 +56,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": {}, + conclusions := pred.Conclusions + if len(conclusions) == 0 { + conclusions = defaultConclusions } predicateResult := common.PredicateResult{ ValuePhrase: "status checks", - ConditionPhrase: "exist and pass", - } - - if pred.Options.SkippedIsSuccess { - predicateResult.ConditionPhrase += " or are skipped" - allowedStatusConclusions["skipped"] = struct{}{} + ConditionPhrase: fmt.Sprintf("exist and have conclusion %s", conclusions.joinWithOr()), } var missingResults []string @@ -83,7 +85,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 +99,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 +109,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 d891a192..7a4cf998 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" From 2d9c41c83e58ded7d2e42e25eed00bf6db0feeee Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Sun, 14 Jul 2024 12:21:31 +0100 Subject: [PATCH 4/8] Drop `CountSkippedStatusAsPassed` which was a leftover --- policy/approval/approve.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 324fd075..58782765 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -48,8 +48,6 @@ type Options struct { RequestReview RequestReview `yaml:"request_review"` - CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"` - Methods *common.Methods `yaml:"methods"` } From 06ae646f577a08c365931527addba40d63ee0e86 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Sun, 14 Jul 2024 12:23:22 +0100 Subject: [PATCH 5/8] README: Add back documentation fo `has_successful_status` Mark it as deprecated instead of removing it. --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index b19f3db9..c350e8eb 100644 --- a/README.md +++ b/README.md @@ -322,6 +322,14 @@ 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: + - "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"]. From 1793921421e208e47ae226e1d93934152b09bc84 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Sun, 14 Jul 2024 13:14:09 +0100 Subject: [PATCH 6/8] Rearrange things to define `HasSuccessfulStatus` in terms of `HasStatus` This is more direct. We want to test `HasSuccessfulStatus` now, so jiggle the testsuite a bit to make that less repetitive by introducing a `StatusTestSuite` struct. --- policy/predicate/predicates.go | 2 +- policy/predicate/status.go | 40 +++++++++---------- policy/predicate/status_test.go | 68 ++++++++++++++++++++++++--------- 3 files changed, 71 insertions(+), 39 deletions(-) diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go index 459cf7b9..eeb9bb53 100644 --- a/policy/predicate/predicates.go +++ b/policy/predicate/predicates.go @@ -33,7 +33,7 @@ type Predicates struct { // `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"` + HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"` HasLabels *HasLabels `yaml:"has_labels"` diff --git a/policy/predicate/status.go b/policy/predicate/status.go index d15131d8..4bbdcbf8 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -40,26 +40,6 @@ func NewHasStatus(statuses []string, conclusions []string) *HasStatus { } } -// 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 { - pred.Statuses = statuses - - return nil - } - - // If that fails, try to unmarshal as the full structure - type rawHasSuccessfulStatus HasStatus - return unmarshal((*rawHasSuccessfulStatus)(pred)) -} - var _ Predicate = HasStatus{} func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { @@ -112,3 +92,23 @@ func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common 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, + Conclusions: allowedConclusions{"success": {}}, + }.Evaluate(ctx, prctx) +} + +func (pred HasSuccessfulStatus) Trigger() common.Trigger { + return common.TriggerStatus +} diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index 7a4cf998..b9d1075a 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -16,9 +16,12 @@ 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" @@ -37,7 +40,12 @@ func keysSorted[V any](m map[string]V) []string { } func TestHasSuccessfulStatus(t *testing.T) { - p := HasStatus{Statuses: []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"} commonTestCases := []StatusTestCase{ { @@ -141,22 +149,34 @@ func TestHasSuccessfulStatus(t *testing.T) { }, } - // 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.Conclusions = allowedConclusions{"success": {}, "skipped": {}} - - for i := 0; i < len(commonTestCases); i++ { - commonTestCases[i].name += ", but skipped statuses are allowed" + 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 i := 0; i < len(okOnlyIfSkippedAllowed); i++ { - okOnlyIfSkippedAllowed[i].name += ", but skipped statuses are allowed" - okOnlyIfSkippedAllowed[i].ExpectedPredicateResult.Satisfied = true + + for _, suite := range testSuites { + runStatusTestCase(t, suite.predicate, suite) } - runStatusTestCase(t, p, commonTestCases) - runStatusTestCase(t, p, okOnlyIfSkippedAllowed) +} + +type StatusTestSuite struct { + nameSuffix string + predicate Predicate + testCases []StatusTestCase + overrideSatisfied *bool } type StatusTestCase struct { @@ -165,10 +185,14 @@ 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 { + 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 @@ -178,7 +202,15 @@ func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) { tc.ExpectedPredicateResult.Values = keysSorted(statuses) } - t.Run(tc.name, func(t *testing.T) { + // `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) From ebee53203ad74705387c4abe080d22ce31fe7f4e Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Sun, 14 Jul 2024 13:34:06 +0100 Subject: [PATCH 7/8] Eliminate the sets It's more code for not too much gain since we're only dealing with tiny slices and not really looking through them that often. --- policy/predicate/predicate.go | 61 --------------------- policy/predicate/predicate_test.go | 87 ------------------------------ policy/predicate/status.go | 39 ++++++++++---- policy/predicate/status_test.go | 42 ++++++++++++++- 4 files changed, 71 insertions(+), 158 deletions(-) diff --git a/policy/predicate/predicate.go b/policy/predicate/predicate.go index cb705e11..15a244a4 100644 --- a/policy/predicate/predicate.go +++ b/policy/predicate/predicate.go @@ -16,9 +16,6 @@ package predicate import ( "context" - "fmt" - "slices" - "strings" "github.com/palantir/policy-bot/policy/common" "github.com/palantir/policy-bot/pull" @@ -30,61 +27,3 @@ type Predicate interface { // Evaluate determines if the predicate is satisfied. Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) } - -type unit struct{} -type set map[string]unit - -// allowedConclusions can be one of: -// action_required, cancelled, failure, neutral, success, skipped, stale, -// timed_out -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) - } - slices.Sort(keys) - - 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{}, -} diff --git a/policy/predicate/predicate_test.go b/policy/predicate/predicate_test.go index a27a4653..b9fff863 100644 --- a/policy/predicate/predicate_test.go +++ b/policy/predicate/predicate_test.go @@ -19,7 +19,6 @@ import ( "github.com/palantir/policy-bot/policy/common" "github.com/stretchr/testify/assert" - "gopkg.in/yaml.v2" ) func assertPredicateResult(t *testing.T, expected, actual *common.PredicateResult) { @@ -28,89 +27,3 @@ func assertPredicateResult(t *testing.T, expected, actual *common.PredicateResul assert.Equal(t, expected.ConditionsMap, actual.ConditionsMap, "conditions were not correct") assert.Equal(t, expected.ConditionValues, actual.ConditionValues, "conditions were not correct") } - -func TestUnmarshalAllowedConclusions(t *testing.T) { - testCases := []struct { - name string - input string - expected allowedConclusions - expectedErr bool - }{ - { - name: "empty", - input: "", - expected: nil, - }, - { - name: "single", - input: `["success"]`, - expected: allowedConclusions{"success": unit{}}, - }, - { - name: "multiple", - input: `["success", "failure"]`, - expected: allowedConclusions{"success": unit{}, "failure": unit{}}, - }, - { - name: "repeat", - input: `["success", "success"]`, - expected: allowedConclusions{"success": unit{}}, - }, - { - name: "invalid", - input: `notyaml`, - expectedErr: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var actual allowedConclusions - err := yaml.UnmarshalStrict([]byte(tc.input), &actual) - - if tc.expectedErr { - assert.Error(t, err, "UnmarshalStrict should have failed") - return - } - - assert.NoError(t, err, "UnmarshalStrict failed") - assert.Equal(t, tc.expected, actual, "values were not correct") - }) - } -} - -func TestJoinWithOr(t *testing.T) { - testCases := []struct { - name string - input allowedConclusions - expected string - }{ - { - name: "empty", - input: nil, - expected: "", - }, - { - name: "one conclusion", - input: allowedConclusions{"success": unit{}}, - expected: "success", - }, - { - name: "two conclusions", - input: allowedConclusions{"success": unit{}, "failure": unit{}}, - expected: "failure or success", - }, - { - name: "three conclusions", - input: allowedConclusions{"success": unit{}, "failure": unit{}, "cancelled": unit{}}, - expected: "cancelled, failure, or success", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - actual := tc.input.joinWithOr() - assert.Equal(t, tc.expected, actual, "values were not correct") - }) - } -} diff --git a/policy/predicate/status.go b/policy/predicate/status.go index 4bbdcbf8..9c0f6879 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -17,6 +17,7 @@ package predicate import ( "context" "fmt" + "slices" "strings" "github.com/palantir/policy-bot/policy/common" @@ -24,18 +25,16 @@ import ( "github.com/pkg/errors" ) +type allowedConclusions []string + type HasStatus struct { Conclusions allowedConclusions `yaml:"conclusions"` Statuses []string `yaml:"statuses"` } func NewHasStatus(statuses []string, conclusions []string) *HasStatus { - conclusionsSet := make(allowedConclusions, len(conclusions)) - for _, conclusion := range conclusions { - conclusionsSet[conclusion] = unit{} - } return &HasStatus{ - Conclusions: conclusionsSet, + Conclusions: conclusions, Statuses: statuses, } } @@ -50,7 +49,7 @@ func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common conclusions := pred.Conclusions if len(conclusions) == 0 { - conclusions = defaultConclusions + conclusions = allowedConclusions{"success"} } predicateResult := common.PredicateResult{ @@ -65,7 +64,7 @@ func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common if !ok { missingResults = append(missingResults, status) } - if _, allowed := conclusions[result]; !allowed { + if !slices.Contains(conclusions, result) { failingStatuses = append(failingStatuses, status) } } @@ -104,11 +103,33 @@ var _ Predicate = HasSuccessfulStatus{} func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) { return HasStatus{ - Statuses: pred, - Conclusions: allowedConclusions{"success": {}}, + 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 b9d1075a..c731dd2b 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -43,7 +43,7 @@ func TestHasSuccessfulStatus(t *testing.T) { hasStatus := HasStatus{Statuses: []string{"status-name", "status-name-2"}} hasStatusSkippedOk := HasStatus{ Statuses: []string{"status-name", "status-name-2"}, - Conclusions: allowedConclusions{"success": {}, "skipped": {}}, + Conclusions: allowedConclusions{"success", "skipped"}, } hasSuccessfulStatus := HasSuccessfulStatus{"status-name", "status-name-2"} @@ -218,3 +218,43 @@ func runStatusTestCase(t *testing.T, p Predicate, suite StatusTestSuite) { }) } } + +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()) + }) + } +} From 605691ab429f12149d2629519cce1b97b0d06e91 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Tue, 16 Jul 2024 10:26:51 +0100 Subject: [PATCH 8/8] Expose `AllowedConclusions` Otherwise people can't directly construct a `HasStatus` --- policy/predicate/status.go | 8 ++++---- policy/predicate/status_test.go | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/policy/predicate/status.go b/policy/predicate/status.go index 9c0f6879..9727d9cf 100644 --- a/policy/predicate/status.go +++ b/policy/predicate/status.go @@ -25,10 +25,10 @@ import ( "github.com/pkg/errors" ) -type allowedConclusions []string +type AllowedConclusions []string type HasStatus struct { - Conclusions allowedConclusions `yaml:"conclusions"` + Conclusions AllowedConclusions `yaml:"conclusions"` Statuses []string `yaml:"statuses"` } @@ -49,7 +49,7 @@ func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common conclusions := pred.Conclusions if len(conclusions) == 0 { - conclusions = allowedConclusions{"success"} + conclusions = AllowedConclusions{"success"} } predicateResult := common.PredicateResult{ @@ -116,7 +116,7 @@ func (pred HasSuccessfulStatus) Trigger() common.Trigger { // 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 { +func (c AllowedConclusions) joinWithOr() string { slices.Sort(c) length := len(c) diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index c731dd2b..952fc1b8 100644 --- a/policy/predicate/status_test.go +++ b/policy/predicate/status_test.go @@ -43,7 +43,7 @@ func TestHasSuccessfulStatus(t *testing.T) { hasStatus := HasStatus{Statuses: []string{"status-name", "status-name-2"}} hasStatusSkippedOk := HasStatus{ Statuses: []string{"status-name", "status-name-2"}, - Conclusions: allowedConclusions{"success", "skipped"}, + Conclusions: AllowedConclusions{"success", "skipped"}, } hasSuccessfulStatus := HasSuccessfulStatus{"status-name", "status-name-2"} @@ -222,32 +222,32 @@ func runStatusTestCase(t *testing.T, p Predicate, suite StatusTestSuite) { func TestJoinWithOr(t *testing.T) { testCases := []struct { name string - input allowedConclusions + input AllowedConclusions expected string }{ { "empty", - allowedConclusions{}, + AllowedConclusions{}, "", }, { "single", - allowedConclusions{"a"}, + AllowedConclusions{"a"}, "a", }, { "two", - allowedConclusions{"a", "b"}, + AllowedConclusions{"a", "b"}, "a or b", }, { "three", - allowedConclusions{"a", "b", "c"}, + AllowedConclusions{"a", "b", "c"}, "a, b, or c", }, { "conclusions get sorted", - allowedConclusions{"c", "a", "b"}, + AllowedConclusions{"c", "a", "b"}, "a, b, or c", }, }