Add has_status predicate (#789)

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 `has_status` predicate that accepts different
conclusions:

    has_status:
      conclusions: ["success", "skipped"]
      statuses:
        - "status 1"
        - "status 2"

Also mark the `has_successful_status` predicate as deprecated, since
`has_status` can do the same thing (and is equivalent when there are no
conclusions specified).

README.md

@@ -322,13 +322,24 @@ 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"].
+  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
   has_labels:
@@ -510,16 +521,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

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"},
+					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{"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{"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{"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{"build"},
+					HasStatus: predicate.NewHasStatus([]string{"status1"}, []string{"skipped", "success"}),
 				},
 			},
 		}

policy/approval/parse_test.go

@@ -35,6 +35,9 @@ func TestParsePolicy(t *testing.T) {
 - and:
   - rule6
   - rule7
+  - or:
+    - rule8
+    - rule9
 `
 
 	ruleText := `
@@ -67,6 +70,15 @@ func TestParsePolicy(t *testing.T) {
       enabled: true
   requires:
     admins: true
+- name: rule8
+  if:
+    has_successful_status:
+      - status1
+- name: rule9
+  if:
+    has_status:
+      conclusions: ["success", "skipped"]
+      statuses: [status2, status3]
 `
 
 	var policy Policy
@@ -119,6 +131,16 @@ func TestParsePolicy(t *testing.T) {
 						&RuleRequirement{
 							rule: rules[6],
 						},
+						&OrRequirement{
+							requirements: []common.Evaluator{
+								&RuleRequirement{
+									rule: rules[7],
+								},
+								&RuleRequirement{
+									rule: rules[8],
+								},
+							},
+						},
 					},
 				},
 			},

policy/predicate/predicates.go

@@ -29,6 +29,10 @@ type Predicates struct {
 
 	ModifiedLines *ModifiedLines `yaml:"modified_lines"`
 
+	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 *HasSuccessfulStatus `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))
 	}

policy/predicate/status.go

@@ -16,37 +16,55 @@ package predicate
 
 import (
 	"context"
+	"fmt"
+	"slices"
 	"strings"
 
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
 )
 
-type HasSuccessfulStatus []string
-
-var _ Predicate = HasSuccessfulStatus([]string{})
+type AllowedConclusions []string
 
-func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
-	statuses, err := prctx.LatestStatuses()
+type HasStatus struct {
+	Conclusions AllowedConclusions `yaml:"conclusions"`
+	Statuses    []string           `yaml:"statuses"`
+}
 
-	predicateResult := common.PredicateResult{
-		ValuePhrase:     "status checks",
-		ConditionPhrase: "exist and pass",
+func NewHasStatus(statuses []string, conclusions []string) *HasStatus {
+	return &HasStatus{
+		Conclusions: conclusions,
+		Statuses:    statuses,
 	}
+}
 
+var _ Predicate = HasStatus{}
+
+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")
 	}
 
+	conclusions := pred.Conclusions
+	if len(conclusions) == 0 {
+		conclusions = AllowedConclusions{"success"}
+	}
+
+	predicateResult := common.PredicateResult{
+		ValuePhrase:     "status checks",
+		ConditionPhrase: fmt.Sprintf("exist and have conclusion %s", conclusions.joinWithOr()),
+	}
+
 	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 !slices.Contains(conclusions, result) {
 			failingStatuses = append(failingStatuses, status)
 		}
 	}
@@ -60,16 +78,58 @@ 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
 	}
 
-	predicateResult.Values = pred
+	predicateResult.Values = pred.Statuses
 	predicateResult.Satisfied = true
 	return &predicateResult, nil
 }
 
+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,
+	}.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
+}