-
Notifications
You must be signed in to change notification settings - Fork 104
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
base: develop
Are you sure you want to change the base?
Add support for allowing skipped checks in has_successful_status
#789
Conversation
Thanks for your interest in palantir/policy-bot, @iainlane! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
I'll square off the CLA question internally as soon as I can, but in the meantime hopefully we can proceed with reviews 😁 |
Thanks for this proposal! Looking at an actual implementation, I'm now thinking this could work better as a new predicate that deprecates # Option 1: same conclusions for all contexts
has_status:
conclusions: ["success", "skipped"]
contexts:
- "status 1"
- "status 2"
# Option 2: conclusions per context
has_status:
"status 1": ["success"]
"status 2": ["success", "skipped"] Internally, we can convert the |
b911ad5
to
f75329e
Compare
Cheers for the review 👍
Sure, that makes sense. It was a bit awkward that we had two forms in one predicate. Take a look at what I've got now? It's fundamentally the same as what I had before, except now:-
We've still got a custom unmarshal function here, which handles unpacking either form into the same struct so they can share the implementation. A bit more stuff has appeared to handle the list of statuses but it's not too bad.
I went for the first one. You can express the second in terms of it as far as I can see, and it feels to me like it'll be more common to want to apply the same conclusions to a whole predicate than select them individually. But I don't feel that strongly about this - it can be changed to the other form easily enough. I've used |
I was just testing this in a real deployment, instead of using the testsuite... and this doesn't work like I thought it did. Given this workflow on:
pull_request:
paths:
- a/**
jobs:
# some jobs here There is nothing reported when the job isn't run due to the path filter not being matched. Or at least nothing that I can find. I think the PR is still useful for conditional jobs, but not conditional workflows, unless there is a way we can see those. |
c22674b
to
451855b
Compare
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.
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: palantir#760
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" ```
451855b
to
d96d2a0
Compare
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:In this mode, we will consider the
skipped
result as acceptable. The current form:remains supported. We have done this by implementing a custom unmarshaling function to be able to handle both forms.
Closes: #760