Add support for allowing skipped checks in `has_successful_status` #789

iainlane · 2024-06-27T19:23:23Z

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:

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:

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

palantirtech · 2024-06-27T19:23:26Z

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.

iainlane · 2024-06-27T19:30:19Z

I'll square off the CLA question internally as soon as I can, but in the meantime hopefully we can proceed with reviews 😁

bluekeyes · 2024-06-28T05:13:45Z

Thanks for this proposal! Looking at an actual implementation, I'm now thinking this could work better as a new predicate that deprecates has_successful_status. This new has_status predicate would let you specify both the contexts and the allowed conclusions:

# 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 has_successful_status predicate in to this new one to avoid duplicating logic. The new predicate also avoids having two forms of a single predicate and allows policies that can work with failure, neutral, and other conclusions. What do you think?

iainlane · 2024-07-01T12:50:46Z

Cheers for the review 👍

Thanks for this proposal! Looking at an actual implementation, I'm now thinking this could work better as a new predicate that deprecates has_successful_status.

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

has_successful_status is kept separate
Any conclusion can be given, instead of only allowing skipped as we did before

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.

# 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"]

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 statuses rather than contexts. That's only because I've not used that word in the context of GitHub CI and I found it bounced off me a bit. But it also doesn't matter too much.

iainlane · 2024-07-05T16:37:18Z

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.

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" ```

bluekeyes · 2024-07-08T18:01:12Z

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.

Yeah, that sounds right. If a workflow trigger doesn't match, GitHub skips doing anything with the workflow. Posting skipped statuses for the jobs probably requires doing at least some evaluation to determine what jobs exist.

It's pretty difficult to account for things like this, since all statuses start out as missing until the reporting system responds to a webhook and posts the initial value. When Policy Bot evaluates a PR, there's no way that I know of to distinguish between a status that will never appear and one that will appear, but just hasn't appeared yet.

If you're able to share more details about what you want to achieve with your policy (maybe on the original issue), I can try to think about alternate approaches that might work.

bluekeyes · 2024-07-08T18:43:34Z

policy/approval/approve.go

@@ -48,6 +48,8 @@ type Options struct {

 	RequestReview RequestReview `yaml:"request_review"`

+	CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"`


I think this is leftover from a previous iteration? It appears unused now

Yes indeed. I've dropped it in 2d9c41c.

bluekeyes · 2024-07-08T18:51:11Z

README.md

-  # "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"


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:

Deprecated: Use `has_status` instead, which is more flexible

Yep sure: 06ae646

bluekeyes · 2024-07-08T19:01:49Z

policy/predicate/status.go

+//     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 {


While this works, it does mean people could use the new has_status predicate by providing only a list of strings or use the old has_successful_status predicate with a conclusions list that doesn't contain success.

Instead, I suggest keeping the old HasSuccessfulStatus type and replacing the Evaluate method with a version that creates a HasStatus object and calls Evaluate on it. That would keep these as two separate predicates for policy parsing purposes, while avoiding most of the code duplication.

Makes sense. Done here: 1793921.

Since we brought the type back, I added it to the testsuite again. Then it started to look a bit unwieldy with all the cases going on, so I reworked that a bit to be hopefully clearer vs. my previous.

bluekeyes · 2024-07-08T19:56:42Z

policy/predicate/predicate.go

+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


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 []string and testing for matching conclusions with slices.Contains?

That could allow duplicates, but duplicates would be obvious in the policy definition and shouldn't cause any problems (other than slightly awkward messages.)

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.

Mark it as deprecated instead of removing it.

This is more direct. We want to test `HasSuccessfulStatus` now, so jiggle the testsuite a bit to make that less repetitive by introducing a `StatusTestSuite` struct.

It's more code for not too much gain since we're only dealing with tiny slices and not really looking through them that often.

iainlane

Thanks for the second look. All the comments should be addressed now. 👍

If you're able to share more details about what you want to achieve with your policy (maybe on the original issue), I can try to think about alternate approaches that might work.

Okey dokey, I'll do that.

iainlane · 2024-07-14T12:38:49Z

policy/approval/approve.go

@@ -48,6 +48,8 @@ type Options struct {

 	RequestReview RequestReview `yaml:"request_review"`

+	CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"`


Yes indeed. I've dropped it in 2d9c41c.

iainlane · 2024-07-14T12:39:12Z

README.md

-  # "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"


Yep sure: 06ae646

iainlane · 2024-07-14T12:40:42Z

policy/predicate/predicate.go

+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


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.

iainlane · 2024-07-14T12:42:07Z

policy/predicate/status.go

+//     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 {


Makes sense. Done here: 1793921.

Since we brought the type back, I added it to the testsuite again. Then it started to look a bit unwieldy with all the cases going on, so I reworked that a bit to be hopefully clearer vs. my previous.

Otherwise people can't directly construct a `HasStatus`

bluekeyes

Looks good, thanks for iterating on this with me. Let me know when you have the CLA sorted out (or if you have any questions about it) and then we can merge this.

iainlane · 2024-07-24T17:18:02Z

Looks good, thanks for iterating on this with me. Let me know when you have the CLA sorted out (or if you have any questions about it) and then we can merge this.

I got the reply from our legal folks earlier. It's good to go and I've signed it.

Thanks - if you could hit the button for me, please. 👍

I'll get to the other PRs and issues tomorrow now I shouldn't be blocking from this side any more.

stevoland · 2024-07-24T21:26:26Z

Thanks so much to both of you, excellent stuff!

bluekeyes · 2024-08-02T02:13:45Z

@iainlane while looking at #807, I realized we may have a bug related to this: the handler for the check_run event, and probably the handler for the status event, exits early when the conclusion of the incoming status is not success. This was an optimization to make fewer requests, but it means we could miss evaluating a policy that requires some other status if there are no other triggering events.

Unfortunately, this could complicate the request optimizations you might be looking at. While we have the "trigger" concept for policies that deal with statuses, it doesn't include the different conclusions, so we may needlessly evaluate policies on pending events when they could only be satisfied by successes.

iainlane · 2024-08-02T09:10:37Z

@iainlane while looking at #807, I realized we may have a bug related to this: the handler for the check_run event, and probably the handler for the status event, exits early when the conclusion of the incoming status is not success. This was an optimization to make fewer requests, but it means we could miss evaluating a policy that requires some other status if there are no other triggering events.

Unfortunately, this could complicate the request optimizations you might be looking at. While we have the "trigger" concept for policies that deal with statuses, it doesn't include the different conclusions, so we may needlessly evaluate policies on pending events when they could only be satisfied by successes.

Ah, yeah, hmm. Good spot.

Would it be acceptable to look for status being completed instead? So we'd only react when runs are finished, but now if they finished with any conclusion.

Or would we be looking at figuring out to get at the set of interesting conclusions in the handler? Feels tricky.

bluekeyes · 2024-08-03T03:13:52Z

Yeah, I think restricting has_status to completed conclusions and then looking for that would be reasonable. If someone has a compelling reason to write a policy that matches on pending statuses in the future, we can revisit at that point.

iainlane mentioned this pull request Jun 27, 2024

Feature Request: Option to count skipped jobs in has_successful_status #760

Closed

iainlane force-pushed the iainlane/has-successful-status-allow-skipped-workflows-to-count-as-passes branch 2 times, most recently from b911ad5 to f75329e Compare July 1, 2024 12:50

iainlane force-pushed the iainlane/has-successful-status-allow-skipped-workflows-to-count-as-passes branch 2 times, most recently from c22674b to 451855b Compare July 7, 2024 11:10

iainlane added 3 commits July 7, 2024 12:12

Add utility functions to allow predicates to take lists of conclusions

caba90e

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.

Rework to expose has_status

d96d2a0

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" ```

iainlane force-pushed the iainlane/has-successful-status-allow-skipped-workflows-to-count-as-passes branch from 451855b to d96d2a0 Compare July 7, 2024 11:13

iainlane mentioned this pull request Jul 7, 2024

feat(predicate): Add has_workflow_result predicate #794

Merged

bluekeyes reviewed Jul 8, 2024

View reviewed changes

iainlane added 4 commits July 14, 2024 12:21

Drop CountSkippedStatusAsPassed which was a leftover

2d9c41c

README: Add back documentation fo has_successful_status

06ae646

Mark it as deprecated instead of removing it.

Rearrange things to define HasSuccessfulStatus in terms of HasStatus

1793921

This is more direct. We want to test `HasSuccessfulStatus` now, so jiggle the testsuite a bit to make that less repetitive by introducing a `StatusTestSuite` struct.

Eliminate the sets

ebee532

It's more code for not too much gain since we're only dealing with tiny slices and not really looking through them that often.

iainlane commented Jul 14, 2024

View reviewed changes

Expose AllowedConclusions

605691a

Otherwise people can't directly construct a `HasStatus`

bluekeyes approved these changes Jul 18, 2024

View reviewed changes

bluekeyes merged commit dd73f68 into palantir:develop Jul 24, 2024
8 checks passed

iainlane deleted the iainlane/has-successful-status-allow-skipped-workflows-to-count-as-passes branch August 2, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for allowing skipped checks in `has_successful_status` #789

Add support for allowing skipped checks in `has_successful_status` #789

iainlane commented Jun 27, 2024

palantirtech commented Jun 27, 2024

iainlane commented Jun 27, 2024

bluekeyes commented Jun 28, 2024

iainlane commented Jul 1, 2024

iainlane commented Jul 5, 2024

bluekeyes commented Jul 8, 2024

bluekeyes Jul 8, 2024

iainlane Jul 14, 2024

bluekeyes Jul 8, 2024

iainlane Jul 14, 2024

bluekeyes Jul 8, 2024

iainlane Jul 14, 2024

bluekeyes Jul 8, 2024

iainlane Jul 14, 2024

iainlane left a comment

iainlane Jul 14, 2024

iainlane Jul 14, 2024

iainlane Jul 14, 2024

iainlane Jul 14, 2024

bluekeyes left a comment

iainlane commented Jul 24, 2024 •

edited

Loading

stevoland commented Jul 24, 2024

bluekeyes commented Aug 2, 2024

iainlane commented Aug 2, 2024

bluekeyes commented Aug 3, 2024

		@@ -48,6 +48,8 @@ type Options struct {

		RequestReview RequestReview `yaml:"request_review"`

		CountSkippedStatusAsPassed bool `yaml:"count_skipped_status_as_passed"`

Add support for allowing skipped checks in has_successful_status #789

Add support for allowing skipped checks in has_successful_status #789

Conversation

iainlane commented Jun 27, 2024

palantirtech commented Jun 27, 2024

iainlane commented Jun 27, 2024

bluekeyes commented Jun 28, 2024

iainlane commented Jul 1, 2024

iainlane commented Jul 5, 2024

bluekeyes commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iainlane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

iainlane commented Jul 24, 2024 • edited Loading

stevoland commented Jul 24, 2024

bluekeyes commented Aug 2, 2024

iainlane commented Aug 2, 2024

bluekeyes commented Aug 3, 2024

Add support for allowing skipped checks in `has_successful_status` #789

Add support for allowing skipped checks in `has_successful_status` #789

iainlane commented Jul 24, 2024 •

edited

Loading