bevyengine
add cancel in progress support to automated CI checks
Objective
When working on PRs, I'll often find that one of the early CI checks fails, and work on fixing the result, but when I push the earlier commits are still being processed by the CI. This would mean that if a new commit is pushed while another CI process is already running on that branch, the first set of jobs will be cancelled - reducing wasted resources and wait time for CI on the latest commits.
Solution
The solution is simply adding Github's concurrency groups to every relevant workflow.
@mockersf @alice-i-cecile - If I'm remembering correctly, you are both very familiar with the Bevy CI system?
This seems sensible, but I haven't worked with this directly before.
this should also not cancel in progress job on a
merge_grouptrigger
Fixed!
Instead of specific exceptions for main and merge_group, it seems like rule we're trying to codify is "only cancel in progress runs for PRs". I think what you have is fine, though it would require updating if there was another branch or type we also don't want to cancel, right. That's not a blocker though, just a note as far as maintenance of this goes.
I can't check if the current one works as expected atm, so I won't approve in this review, but the code does look correct to me (untested).
done!
I'm officially punting the decision on this to @mockersf. I don't feel strongly enough either way.
@mockersf - I know you don't like this idea, so feel free to close this. But I did want to write out the benefits I was looking for with this:
When working on a PR, I often find myself committing, pushing, and then catching a mistake, or seeing that one part of the worfklow failed already and I can fix it. So I'll go and fix the issue, and re-push, but then need to wait for the entire CI process on the original, known to be broken, commit before I can see if the new one passes CI.
The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it.
And since it's limited to commits within PRs, and PRs need to be passing to get merged, the lost information is about breaks that are already accounted for and fixed by the time it's merged into main.
The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it.
I'm convinced by this argument. Let's give it a go.
So I'll go and fix the issue, and re-push, but then need to wait for the entire CI process on the original, known to be broken, commit before I can see if the new one passes CI.
The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it.
This was already the case, multiple CI runs can run in parallel for the same PR
So I'll go and fix the issue, and re-push, but then need to wait for the entire CI process on the original, known to be broken, commit before I can see if the new one passes CI. The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it.
This was already the case, multiple CI runs can run in parallel for the same PR
Thanks for the correction.
So I'll go and fix the issue, and re-push, but then need to wait for the entire CI process on the original, known to be broken, commit before I can see if the new one passes CI. The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it.
This was already the case, multiple CI runs can run in parallel for the same PR
I know this is already merged and I don't mean to beat a dead horse, but, for posterity, I thought @lee-orr's point was to say:
"The benefit this provides lives primarily there - it ensures that if I catch a broken commit early, I can push a new commit to fix it and the previous runs will be canceled and won't unnecessarily clog up Bevy's CI."
I thought they were not trying to make the point about being able to run multiple runs at the same time on a PR, but that it's about canceling runs on a PR you don't care about anymore.
Yup, especially since only 20 jobs can be run concurrently across the entire Bevy org. Cancelling jobs when a run has already failed frees up room for new jobs. See this comment for further details.
