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

@@ -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

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"}),
 				},
 			},
 		}

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

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))
 	}

policy/predicate/status.go

@@ -16,31 +16,91 @@ package predicate
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"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": {},
+	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 +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
 }

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"