GitHub Community
Required actions workflows from branch protections should only be required if run #13690
Replies: 36 comments 32 replies
-
|
Thanks @daltonv. I agree that this is not the ideal solution. I'll talk with the team that owns branch protection to give them your feedback. I'll also talk to the docs team to suggest some improvements for this article in the meantime. Thanks again for taking the time to leave feedback. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @ethomson, while looking for more info I just realized I duplicated this discussion. But I'm sharing mine to add more context on other scenarios which make the actual workaround very messy https://github.com/orgs/community/discussions/44490#discussion-4761945 I'm specifically bringing this link out, just so you (and others) can realize how much burden the recommended workaround from the documentation adds to the pipelines: https://github.com/ZcashFoundation/zebra/tree/main/.github/workflows |
Beta Was this translation helpful? Give feedback.
-
|
This should be a generalized change as it affects users who don't use GitHub Actions, but instead have third party CI/CD integrations triggered via webhooks. The option should simply be |
Beta Was this translation helpful? Give feedback.
-
|
This is the behaviour I expect too! And having to explicitly manage the checks to pass in the GUI is redundant and bug-prone. I want to manage everything by code. |
Beta Was this translation helpful? Give feedback.
-
|
Just chiming in and it's kind of shocking there's no way to do this - if a task runs I expect it to either pass or at least have the option of blocking merges without having to itemize every task. Having to itemize them creates a security gap and as mentioned above is problematic when dealing with conditional tasks. |
Beta Was this translation helpful? Give feedback.
-
|
This is the only sane behavior any human could think of. How come it's implemented otherwise? Who would want to specify those checks ahead of time in the repo settings? Why? Just why? This makes this feature utterly useless I believe. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed. I can't believe this isn't the first thing that was implemented around requiring github actions to pass in branch protections. There 100% should be a checkbox that allows you to require any triggered workflow to pass. |
Beta Was this translation helpful? Give feedback.
-
|
See also the most active discussion in ye olde GitHub Community Forum on this topic: https://github.community/t/feature-request-conditional-required-checks/16761 |
Beta Was this translation helpful? Give feedback.
-
|
Link seems to be broken |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
I think is either the same one or one equivalent. |
Beta Was this translation helpful? Give feedback.
-
|
See also https://github.community/t/feature-request-required-checks-conditioned-on-path/2042 |
Beta Was this translation helpful? Give feedback.
-
|
Where did this page go? I've found it in a lot of places, but now it just redirects to the GitHub Community landing page. |
Beta Was this translation helpful? Give feedback.
-
|
Ran into it as well. Having something like this would also let us implement more straightforward skipping. For instance, I don't need to to spend the time running all our test, visual regressions, etc. on a PR from an internal bot that edits changelogs, but I absolutely want those to be required checks for us humans so we can use really helpful things like auto-merging, etc. |
Beta Was this translation helpful? Give feedback.
-
|
Very much want this, we spend piles of cash on GitHub because of this shortcoming. |
Beta Was this translation helpful? Give feedback.
-
|
+1. We have the same problem over at Keycloak: Multiple workflows / jobs are running for e.g. a simple typo in our documentation, or a bump in npm dependencies. Would be very nice to have better support than the dummy workflow approach. |
Beta Was this translation helpful? Give feedback.
-
|
Running into similar problem when using conditional workflow via circle-ci. It should not be waiting infinitely if the workflow doesn't run at all |
Beta Was this translation helpful? Give feedback.
-
|
+1, it's surprising that GitHub and disappointing that GitHub doesn't fully support a code first approach for the definition of requirements. The dummy workaround is hacky to say the least. |
Beta Was this translation helpful? Give feedback.
-
|
+1 |
Beta Was this translation helpful? Give feedback.
-
|
+1 |
Beta Was this translation helpful? Give feedback.
-
|
this is a real pain. any update on this? @ethomson |
Beta Was this translation helpful? Give feedback.
-
|
Hard not to see this as a cash grab, we've been asking for it for 3 years now, the only way to safely protect branches in this scenario is to run all checks always. |
Beta Was this translation helpful? Give feedback.
-
|
Came here to add to the chorus of programmers asking for help with this sad trombone UX. |
Beta Was this translation helpful? Give feedback.
-
|
I got the "dummy workflow workaround" working but it broke after about a week. I don't know if it was some internal Github change or if my setup is just fragile, but the whole situation is pretty disappointing nonetheless. |
Beta Was this translation helpful? Give feedback.
-
|
@daltonv You wrote 'Instead it should be changed…' but it obviously they haven't yet. Perhaps you'd like to un-mark this as 'answered' if possible? |
Beta Was this translation helpful? Give feedback.
-
|
I'm not quite sure what the policy is regarding what constitutes an answer, but hey if this gets more visibility by GitHub as an unanswered thread then why not. |
Beta Was this translation helpful? Give feedback.
-
|
I have a workflow that does something when a PR is opened or closed. I have implemented this as a required workflow to apply it to all repositories in the organization. I arbitrarily have to also trigger it on PR 'synchronize' and 'reopened' event types and just skip all the steps. I agree it would make more sense to have required workflows only block the PR if they actually satisfy the trigger conditions. |
Beta Was this translation helpful? Give feedback.
-
|
How does that solve the problem described in the discussion? |
Beta Was this translation helpful? Give feedback.
-
|
I did a writeup on how we are working around this at Mixpanel: Enforcing required checks on conditional CI jobs in a GitHub monorepo Would love to update the blog post with a big banner saying "disregard this post, everything in GitHub works as expected now!" 👍 |
Beta Was this translation helpful? Give feedback.
-
|
This is off-topic, but it's pretty cool that your name is Austin and you live in Austin 😄 |
Beta Was this translation helpful? Give feedback.
-
|
I hate to be that guy, but: any official update on this? Maybe this made sense in some convoluted archaic GitFlow with 6 long-lived branches that each have their own branch protection rules, but the world is doing much less of that these days. I looked into the new Repository Rules (in beta as of this writing) thinking GitHub might have been focusing their efforts there, but it seems (so far) that required checks work the same way there. |
Beta Was this translation helpful? Give feedback.
-
|
Update: back in April 2023, in the Repository Rules Public Beta Feedback, this thread was mentioned and looks like in the works within the upcoming Repository Rules (which will eventually replace the branch protection rules), so that's good to see.
|
Beta Was this translation helpful? Give feedback.
-
|
As of today, ruleset's don't handle this any better. They still just wait forever for statuses from jobs that are triggered by workflows with path conditions. Still waiting to see if they're working toward the |
Beta Was this translation helpful? Give feedback.
-
|
+1 for |
Beta Was this translation helpful? Give feedback.
-
|
I really still can't get my head around how this is being implemented. Where is the |
Beta Was this translation helpful? Give feedback.
-
|
+1 is there any plan in place for this? |
Beta Was this translation helpful? Give feedback.
-
|
I ended up having to write a GitHub Action to add a status check that polls other GitHub check runs (but not from 3rd parties). Then marking name: summary
on:
pull_request:
jobs:
enforce-all-checks:
runs-on: ubuntu-latest
permissions:
checks: read
steps:
- name: GitHub Checks
uses: poseidon/wait-for-status-checks@v0.1.0
with:
token: ${{ secrets.GITHUB_TOKEN }}It works, but writing actions is fundamentally limited compared to native GitHub support. First, we have to poll GitHub which is wasteful. Next, it's possible (though unlikely) to race if no other GitHub Checks start for a whle. And finally, when a triggered check fails, developers naturally re-run it, but that can't reasonably re-trigger a job like |
Beta Was this translation helpful? Give feedback.
-
|
good idea 👍just tried it in our large monorepo with multiple workflows. unfortunately it looks like it doesn't wait for all workflows to complete. most likely it's due to the API call which returns only the first 30 checks by default. opened an issue on the repo: poseidon/wait-for-status-checks#8 |
Beta Was this translation helpful? Give feedback.
-
|
Nice! the perfect solution for me. Easy and gets work done. |
Beta Was this translation helpful? Give feedback.
-
|
My approach for this has been a reusable workflow named "ready to merge" that parses the |
Beta Was this translation helpful? Give feedback.
-
|
I think another caveat to this approach is that you can only use |
Beta Was this translation helpful? Give feedback.
-
|
Oh, and to require "ready to merge" that workflow needs to always be triggered. Otherwise, if the whole workflow only runs for changes to And with multiple workflows, you'd need different job names "ready to merge 1", "ready to merge 2". I think your approach is more geared for having a finalizer job at the end of a workflow |
Beta Was this translation helpful? Give feedback.
-
|
We ran into issues using a workflow to drive the "ready to merge" functionality. @dghubble pointed out a few that we also ran into but in addition:
We ended up creating an internal GitHub App rather than using an action. There's other advantages to having full control of the lifecycle as a real app rather than a gha workflow, speed and being able to control api rate limit usage etc. |
Beta Was this translation helpful? Give feedback.
-
|
@dghubble yep, those caveats are mentioned in the link. I did start off originally having multiple workflows, but with the move over to reusable workflows it was easier just to have a single "on pull request" workflow that ties things together with "ready to merge" at the end. The need for a workaround at all isn't ideal though :-( @austinpray-mixpanel the merge group emails are a bit of a mess across the board, but if you've got things all defined in a single workflow, the reason for failure is a bit more obvious:
...I've not had the need to re-trigger single failed jobs, but are you saying that the dependent jobs are not automatically re-triggered? |
Beta Was this translation helpful? Give feedback.
-
|
👋 First thanks for the feedback and discussions on work arounds. This behavior is a long standing one that exists for reasons (no need to bore y'all with history on things we should fix.) We've working on the successor to branch protections in Repository Rules and are making strides to have those interoperate with Actions with Required Workflows. This will help us close gaps between "branch protections" and actions, but not this one yet. That said we do have an internal issue tracking this request. A question for y'all to help us build internal context, what are the typical path filters y'all are using? Is there a specific path/file type etc? Is this limited to only actions workflows or are there areas where path filtering would be useful? |
Beta Was this translation helpful? Give feedback.
-
|
+1 for monorepos combined with sparse checkouts Service specific directories like "Only run marketing site build if you touch the marketing site" pull_request:
paths:
- '.github/actions/setup-node/**'
- '.github/workflows/marketing-site.yaml'
- '.github/workflows/marketing-site-build-and-upload.yaml'
- 'marketing-site/**'In our monorepo we have multiple languages floating around so we only run the respective linter if the language is touched Python on:
pull_request:
paths:
- 'tox.ini'
- '**.py'
- '.github/actions/setup-gcp/**'
- '.github/workflows/lint-python.yaml'
- 'google-cloud/container/python-lint/pin'
- 'tools/flake8'
- 'tools/black'
- 'dagger/lib/images.json'Javascript pull_request:
paths:
- '.browserslistrc'
- '.editorconfig'
- '.eslintignore'
- '.github/**'
- '.npmrc'
- '.prettierignore'
- '.prettierrc'
- '*.js'
- '*.json'
- 'configurations/devs/**'
- 'dagger/**/*.json'
- 'iron/**'
- 'media/**'
- 'tests/browser/**'
- 'tools/webapp_smoke_tests/**'
- 'webapp/app_api/**/*.ts'
- 'webpack/**'
- 'yarn.lock'
- '!marketing-site/**' |
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight, my usecase is even simpler - I want all my workflows to run on every PR. Some of those workflows might have conditional logic in the |
Beta Was this translation helpful? Give feedback.
-
|
I agree with @tylerlaprade. The existing action trigger filters are more than sufficient for my use case, I just want to be able to say "allow merging only if actions have passed, but ignore actions that have been excluded due to filters". |
Beta Was this translation helpful? Give feedback.
-
|
@paulgb yup that's the exact use case we have. I wrote up how we are working around it by implementing a GitHub app: https://engineering.mixpanel.com/enforcing-required-checks-on-conditional-ci-jobs-in-a-github-monorepo-8d4949694340 |
Beta Was this translation helpful? Give feedback.
-
|
agree with @tylerlaprade, I wanted to add that there's not only the |
Beta Was this translation helpful? Give feedback.
-
The most typical use-case is monorepo with frontend/backend/packages, where the only subset of flows that we want to run is what was affected by the PR. Hence paths are something like |
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight any updates on this? almost half a year later and no news :( |
Beta Was this translation helpful? Give feedback.
-
|
We got an AMAZING workaround by Paul from GitHub enterprise support. Unfortunately he hasn't posted it here himself even though he promised to, roughly two weeks ago. He understands it better and wanted to package it nicer, but this is it: It is a standalone workflow that polls the github API every minute for other workflows running for the same merge request and first completes once none is in progress anymore. How to:
name: Confirm that all checks have passed or skipped
on:
pull_request:
types: [opened, synchronize, reopened]
branches:
- main
jobs:
check_commit_status:
runs-on: ubuntu-latest
steps:
- name: Confirm Check Runs
uses: actions/github-script@v7
with:
script: |
console.log("Sleeping 10s to give other runs a chance to start");
await sleep(10000);
const { owner, repo } = context.repo;
const commit_sha = context.payload.pull_request.head.sha; // Get the SHA of the head commit of the PR
const maxRetries = 15; // Maximum number of retries
const excludedCheckRunName = 'check_commit_status' // important you don't define name key for the job
let retryCount = 0;
async function fetchCheckRuns() {
const response = await github.rest.checks.listForRef({
owner,
repo,
ref: commit_sha,
per_page: 100
});
return response.data.check_runs;
}
async function sleep(ms) {
return new Promise(resolve => setTimeout(resolve, ms));
}
let checkRuns = await fetchCheckRuns();
let inProgressFound = false;
let failureFound = false;
do {
inProgressFound = false;
failureFound = false;
for (const checkRun of checkRuns) {
// exclude this job from the evaluation
if (checkRun.name === excludedCheckRunName) {
console.log(`Skipping excluded check run: ${checkRun.name}`);
continue;
}
if (checkRun.status === 'in_progress') {
inProgressFound = true;
console.log(`Check run in progress: ${checkRun.name}`);
}
if (checkRun.conclusion === 'failure') {
failureFound = true;
console.log(`Check run failed: ${checkRun.name}`);
}
}
if (inProgressFound) {
if (retryCount >= maxRetries) {
core.setFailed('Maximum retries reached with check runs still in progress.');
break;
}
console.log('Waiting for 1 minute before rechecking...');
await sleep(60000);
checkRuns = await fetchCheckRuns();
retryCount++;
}
} while (inProgressFound);
if (failureFound) {
core.setFailed('There are failed check runs.');
}
Edit: if you want to save github actions minutes by first starting the workaround workflow after some minutes, you can add
|
Beta Was this translation helpful? Give feedback.
-
|
I think there's still a big issue with this script, if commit 1 triggers X and Y workflows, both of which fail, and then commit 2 triggers Z workflow, this status check will pass. |
Beta Was this translation helpful? Give feedback.
-
|
@timos-flwls , that's an interesting scenario. I am writing a GitHub Actions for the workaround and that is a scenario I had not thought about. So, in your case, would you want to walk through all the commits in the specific pull request from latest to earliest and confirm that the check passed? I am curious on how you would end up with commit 1 triggering X and Y and the same workflows are not triggered on commit 2 given how GitHub determines the diff comparison for the paths filters:
In your, case, are you considering a scenario where the base branch is merged to the pull request branch (topic branch)? |
Beta Was this translation helpful? Give feedback.
-
Ah that's interesting and might explain once when a MR got merged before all workflows had passed. So you're saying that the second commit doesn't trigger the "check_commit_status"-workflow again? Why not? |
Beta Was this translation helpful? Give feedback.
-
|
I've created a GitHub Action that wraps the logic of what @danieltroger described in a nicer package. Check it out at https://github.com/wechuli/allcheckspassed. It provides some additional options such as excluding certain checks from the evaluation and choosing what exactly constitutes a passing check (e.g. skipped, neutral). The action also creates a summary of the checks that were evaluated. Additional documentation is on the README. Ultimately this is only a workaround for a missing feature, the main downside it has is that you need to configure it on its own workflow and it will consume your Actions minutes, I have tried to document this under limitations section. Let me know what you think, if you want some features it doesn't have, please open an issue (or a pr 😉 ). |
Beta Was this translation helpful? Give feedback.
-
|
This is great, thanks for making this! |
Beta Was this translation helpful? Give feedback.
-
|
Just a note that https://github.com/palantir/policy-bot is a great option until github support this. Only real downside is the duplication of conditions between the policies and the conditional jobs imo. |
Beta Was this translation helpful? Give feedback.
and others
-
If you have branch protections set so that a GitHub Action workflow must be run and pass before you can merge, and this workflow has path filtering in the
on:clause then you sometimes can't ever merge because the workflow doesn't run.This GitHub docs talk about this issue here: https://docs.github.com/en/enterprise-cloud@latest/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/troubleshooting-required-status-checks#handling-skipped-but-required-checks
There is a workaround suggested in the doc article, but it involves making a "dummy" workflow of the same name of the required workflow. This dummy workflow always passes and is triggered by the inverse path filtering of the required workflow.
This isn't the best solution in my mind because:
Instead it should be changed so local workflows are only required if they run in the first place.
Beta Was this translation helpful? Give feedback.
All reactions