Fixed CI Cancelling Workflows On Past Default Branch Commits #2896
Conversation
dimitribouniol
changed the title
.github/workflows/soundness.yml
Outdated
| ## We are cancelling previously triggered workflow runs | ||
| ## Cancel previous runs for the same PR, unless the workflow is running on the main branch, then don't cancel it so we have historical status checks. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }}-soundness | ||
| cancel-in-progress: true | ||
| group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref != 'refs/heads/main' || github.run_number }}-soundness | ||
| cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }} |
There was a problem hiding this comment.
What's the point of the group anymore if you're going to make everything (every PR run, and every push on main) have different groups?
If you just want to keep main branch events always running till the end, then you can use something like:
${{ github.repository.default_branch == github.ref_name }}
github.repository.default_branch is available in most events including pushes and PRs and looks like main, and github.ref_name will also resolve to 'main' (not available in PR events).
In PRs, you'll need to use github.head_ref to get the proper head branch name.
So generally ${{ github.head_ref || github.ref_name }} will give you the proper branch name in both PRs and push events.
There was a problem hiding this comment.
What's the point of the group anymore if you're going to make everything (every PR run, and every push on main) have different groups?
I'd say my ideal setup is one group per PR, and one group per commit on main. I must admit I copied this from an old repo (3-4 years ago?) where we had trouble initially without the run number, but you are right that we probably don't want it here 😅. I'll update accordingly for the default branch.
There was a problem hiding this comment.
I just realized what I meant to write is
github.event.repository.default_branch
Not
github.repository.default_branch
The second one will not work 😅
There was a problem hiding this comment.
Good to know, though no longer necessary since it's been moved only to files that need them 😅
dimitribouniol
force-pushed
the
dimitri/ci-concurrency
branch
from
844ec13
to
8ef07a4
Compare
dimitribouniol
changed the title
dimitribouniol
force-pushed
the
dimitri/ci-concurrency
branch
from
8ef07a4
to
93f41ec
Compare
.github/workflows/soundness.yml
Outdated
| ## We are cancelling previously triggered workflow runs | ||
| ## Cancel previous runs for the same PR, unless the workflow is running on the main branch, then don't cancel it so we have historical status checks. | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }}-soundness | ||
| cancel-in-progress: true | ||
| cancel-in-progress: ${{ github.repository.default_branch != github.ref_name }} |
There was a problem hiding this comment.
One thing that I came across recently is that we can define the concurrency on the call of a reusable workflow. I think we should explore doing that instead of defining it here. This would allow us to just not configure it for the main and scheduled workflows.
There was a problem hiding this comment.
Do you mean in the calling repo's main workflow yaml files? That's where I was assuming they'd go — I was actually surprised to see them defined here. If we prefer that approach, I can move these to pull-request.yml and pull-request-label.yml, and let other repos that depend on these do the same.
There was a problem hiding this comment.
Not as a top level concurrency definition since that didn't work but as a job level concurrency. Would be interested to try that out
There was a problem hiding this comment.
Curious to hear why a top level concurrency definition didn't work (at least in a file like pull-request.yml) — that's always been where I've put mine. Happy to try either though, it shouldn't matter too much.
There was a problem hiding this comment.
(Also, this brings up the question of if we need to pass a name to all of these, since it's no longer being used for this purpose (I'll double check the rest of the workflows to double check)
There was a problem hiding this comment.
The top level concurrency definitions do work but it makes it more complicated since you gotta pass in workflow inputs for these reusable workflows that influence the concurrency group names. We can probably use GitHub context here instead but I was interested to see if it just easier if we define them when calling a reusable workflow.
It might be that it's not. I just wanted to see it tried out.
There was a problem hiding this comment.
I guess I meant top-level as in the workflow file that calls all the other workflow files (ie. on pull-request). I'd like to propose this, actually, as it makes it more obvious that concurrency cancellation is in fact enabled, there is no need to pass extra parameters to inform the groups, and it becomes defined in a single place. Let me push a version that shows this.
Only downside is every repo will need to copy the three lines that describe the concurrency group, but as mentioned, I think this clarifies things more than anything. What do you think?
dimitribouniol
force-pushed
the
dimitri/ci-concurrency
branch
from
93f41ec
to
4800939
Compare
dimitribouniol
force-pushed
the
dimitri/ci-concurrency
branch
from
4800939
to
ff8843c
Compare
Fixed an issue where workflows running for the latest commit on main would cancel past those running on just-merged commits.
Motivation:
I noticed while updating CI in other swift-server repos that some of these workflows would cancel for commits just merged on main.
Modifications:
This will only cancel PR-related workflows, which means those running on main will be allowed to always finish, maintaining historical status checks.
Result:
Hopefully, workflows that cancel each other less often.