feat(predicate): Add has_workflow_result predicate
#794
Conversation
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.
|
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
force-pushed
the
iainlane/workflow-run
branch
from
359e20d
to
f1f794b
Compare
This is a new predicate which matches on the results of an entire GitHub Actions workflow. This is similar to `has_status`, except this matches on statuses only, which for GitHub Actions are roughly equivalent to _jobs_ in a Workflow. The workflows are given as paths in the repository, as these can't be dynamic and are easy to predict when writing policies.
iainlane
force-pushed
the
iainlane/workflow-run
branch
from
f1f794b
to
d0d510d
Compare
There was a problem hiding this comment.
Thanks for proposing this. I haven't used Actions enough to run into this problem, but it seems reasonable to have an Actions-specific predicate to simplify things.
Since this contains some overlap with #789, my comment from there about the allowedConclusions set also applies here. I think we'll want to pick one of these PRs to focus on and merge first, and then update the other one to resolve conflicts.
| @@ -26,6 +26,10 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| // GitHubAppID is the GitHub Actions App ID. When a check suite is created | |||
| // by GitHub Actions, its `app.id` field is set to this value. | |||
| GitHubAppID = 15368 | |||
There was a problem hiding this comment.
If we have to use this (which might not be necessary if we switch to the REST API), it will need to be configurable. For example, on one of our internal GitHub Enterprise instances, the App ID for the github-actions app is 152.
Another alternative might be to filter based on the app name (which should be consistent) instead of the ID.
| // TODO: We could use the REST API to get the path, once | ||
| // https://github.com/google/go-github/commit/7665f317d3985efe6a7828e3af2751c41b3526b1 | ||
| // is in a release. |
There was a problem hiding this comment.
Looking at the APIs, I think the REST API makes this more straightforward, so it might be worth copying the relevant function and the partial struct we need in to this project while waiting for a release.
https://github.com/palantir/policy-bot/blob/67b611ab74ebfd63f98addb6105e06a71e65bb7f/pull/github_teams.go is an example of where we did something similar previously when an API we still used was removed upstream.
| } | ||
| } `graphql:"commits(last: 1)"` | ||
| } `graphql:"pullRequest(number: $number)"` | ||
| } `graphql:"repository(owner: $owner, name: $repo)"` |
There was a problem hiding this comment.
If we stick with GraphQL, I think this query can be simplified by using the object query on Repository to directly get the head commit by SHA. This is also important for consistency - we want to make sure the commit we think we're evaluating is the one we actually get the check suites from.
|
|
||
| if len(missingResults) > 0 { | ||
| predicateResult.Values = missingResults | ||
| predicateResult.Description = "One or more workflow runs are missing: " + strings.Join(missingResults, ", ") |
There was a problem hiding this comment.
| predicateResult.Description = "One or more workflow runs are missing: " + strings.Join(missingResults, ", ") | |
| predicateResult.Description = "One or more workflow runs are missing: " + strings.Join(missingResults, ", ") |
| - ".github/workflows/a.yml" | ||
| - ".github/workflows/b.yml" |
There was a problem hiding this comment.
Since workflows must exist in the .github/workflows directory, would it make sense to exclude that from these paths? Or do you think it is clearer to specify the full path?
|
Cheers. I'll mark this as a draft until #789 is good to go, and then pivot over here, so we don't have too much in flight at once. |
This is a new predicate which matches on the results of an entire GitHub Actions workflow.
This is similar to (doesn't replace, both are complementary)
has_statusfrom #789, excepthas_statusmatches on statuses or check runs, which for GitHub Actions are basically equivalent to jobs in a Workflow.has_workflow_resultfinds the check suite for the PR and reports its conclusion, which is the conclusion of the whole workflow run - of all of its jobs.With
has_workflow_result, the workflows are given as paths in the repository, as these can't be dynamic and are easy to predict when writing policies, as well as easy to review for correctness.With this, you can write
and then we'll wait for all the jobs in this workflow to succeed (or with the optional
conclusions, to conclude with whatever you want), which is more convenient than listing all the job names out. This new predicate is really to make writing rules easier. At least for the workflows I use daily it's much more common to want to wait for every job than just a subset of them. 馃檪For the avoidance of doubt, it doesn't solve the problem I mentioned in #789 where we don't see completely skipped jobs (if the
onfilters resulted in a skip).