Skip to content

Commit

Permalink
Add support for allowing skipped checks in has_successful_status
Browse files Browse the repository at this point in the history
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
  • Loading branch information
iainlane committed Jun 27, 2024
1 parent beb44ef commit 7aaacbf
Show file tree
Hide file tree
Showing 6 changed files with 168 additions and 16 deletions.
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions policy/approval/approve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}

Expand Down
10 changes: 5 additions & 5 deletions policy/approval/approve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}},
},
},
}
Expand All @@ -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"}},
},
},
}
Expand All @@ -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"}},
},
},
}
Expand All @@ -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"}},
},
},
}
Expand Down Expand Up @@ -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"}},
},
},
}
Expand Down
23 changes: 23 additions & 0 deletions policy/approval/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ func TestParsePolicy(t *testing.T) {
- and:
- rule6
- rule7
- or:
- rule8
- rule9
`

ruleText := `
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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],
},
},
},
},
},
},
Expand Down
51 changes: 44 additions & 7 deletions policy/predicate/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand All @@ -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
}
Expand Down
86 changes: 82 additions & 4 deletions policy/predicate/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package predicate

import (
"context"
"slices"
"testing"

"github.com/palantir/policy-bot/policy/common"
Expand All @@ -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{
Expand All @@ -38,7 +50,6 @@ func TestHasSuccessfulStatus(t *testing.T) {
},
&common.PredicateResult{
Satisfied: true,
Values: []string{"status-name", "status-name-2"},
},
},
{
Expand Down Expand Up @@ -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{},
Expand All @@ -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 {
Expand All @@ -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") {
Expand Down

0 comments on commit 7aaacbf

Please sign in to comment.