[ci] Cancel the pending PR builds on a new push. #14445
Conversation
|
Starting build on |
.github/workflows/root-ci.yml
Outdated
| OS_REGION_NAME: 'cern' | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
As is this setting will apply to all the workflow (see similar discussion at https://github.com/celeritas-project/celeritas/pull/1020/files#r1399421620). Is that what we want?
There was a problem hiding this comment.
I am not sure what was discussed there and in which context but this change stops the current ongoing build of a PR upon push and starts a new one. I think that’s what we want most of the time and that’s consistent to our jenkins infrastructure.
We might want to limit that to PRs only.
There was a problem hiding this comment.
It also applies to branches where we do not want to cancel builds. There was a lengthy discussion on Mattermost with the preliminary outcome that it's hard to implement cancellation only for PRs...
There was a problem hiding this comment.
I just noticed that GitHub updated their documentation and provides a recipe to implement what we need: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-a-fallback-value
So what we want is probably:
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
cancel-in-progress: true
The alternative with cancel-in-progress: ${{ github.event_name == 'pull_request' }} probably also works, but I don't know what are the consequences of using the undefined github.event.pull_request.number on branches and putting all workflow runs into a single group...
There was a problem hiding this comment.
If you are sure that what you suggest is better - I can put it in.
There was a problem hiding this comment.
So I just checked both options in https://github.com/hahnjo/github-actions-test: They work equally well for PRs, but the consequence of putting all non-PR workflows into a single concurrency group means that only one of them is running at a time. For example, https://github.com/hahnjo/github-actions-test/actions/runs/7722867934 was pending while the previous https://github.com/hahnjo/github-actions-test/actions/runs/7722862441 was still running. That's a problem because it mixes together all branches and other ways of triggering a workflow (scheduled nightly, release), and even worse because of our heterogeneous runners and we want fast Linux runners to start building the next commit while Windows and macOS is still working on (multiple) others.
tl;dr I think group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} gives us what we want, at least in the scenarios I tested.
Test Results 10 files 10 suites 1d 23h 39m 12s ⏱️ Results for commit 479db0c. ♻️ This comment has been updated with latest results. |
|
Starting build on |
Co-authored-by: Jonas Hahnfeld <[email protected]>
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests: |
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
Thanks! Please squash on merge, we don't need the individual commits in the history.
We should only have squash and merge allowed to keep our history friendly to git bisect... |
I disagree, in many cases it's helpful to have more detailed and logically separate commits and for example I try to make sure that all of them build. Also, merge commits don't help all that much for bisect, you might still end up in the branch and you have to manually figure out that you're supposed to skip it... |
No description provided.