-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for allowing skipped checks in has_successful_status
#789
Changes from 3 commits
caba90e
a5acfbc
d96d2a0
2d9c41c
06ae646
1793921
ebee532
605691a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,8 @@ type Options struct { | |
|
||
RequestReview RequestReview `yaml:"request_review"` | ||
|
||
CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is leftover from a previous iteration? It appears unused now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes indeed. I've dropped it in 2d9c41c. |
||
|
||
Methods *common.Methods `yaml:"methods"` | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious for your opinion here: there's a decent amount of code in this PR for converting between this set representation and a list of strings in different circumstances. Given that the number of elements in the set is almost always going to be less than 3, what do you think about using a simple That could allow duplicates, but duplicates would be obvious in the policy definition and shouldn't cause any problems (other than slightly awkward messages.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah no worries. I'm used to writing such code so I tend to reach for it but it's fine to skip it in this case. See ebee532. If needed we could dedupe when printing, when looking at the slice or any other time. But I doubt it'll be a big deal since we're talking about a small number of possible values. |
||
|
||
// 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{}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the benefit of people with older policies, I'd like to keep the documentation for this predicate, but with a note that it is deprecated:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sure: 06ae646