Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: cancel in-progress runs when new changes are pushed up #2997

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Apr 7, 2024

I personally think this is a good idea especially given this projects CI involves over 250+ jobs but I can understand if someone has an alternative workflow that makes it undesirable so feel free to close if you're not a fan 🙂

Related docs: https://docs.github.com/en/actions/using-jobs/using-concurrency

Copy link

codecov bot commented Apr 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.87%. Comparing base (8587c85) to head (17223d4).

❗ Current head 17223d4 differs from pull request most recent head f77ceb6. Consider uploading reports for the commit f77ceb6 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2997       +/-   ##
===========================================
+ Coverage   82.45%   95.87%   +13.42%     
===========================================
  Files          86       78        -8     
  Lines        3813     3275      -538     
  Branches     1284     1150      -134     
===========================================
- Hits         3144     3140        -4     
+ Misses        669      135      -534     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ljharb
Copy link
Member

ljharb commented Apr 7, 2024

On branch pushes, it's critical that a new push not cancel an existing run. I'm fine with this change if it can be restricted to PR runs.

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 7, 2024

hmm I'm not sure if that's possible, at least with concurrency - you should be able to include the trigger type in the grouping to ensure e.g. pull_request triggered workflows won't cancel push ones, but that would still mean in theory you could cancel a push-based workflow; though I'm not sure if that would actually be possible if its keyed to ref as well because that should change on every push? 🤔

Could you expand on why its critical that branch pushes are not cancelled by pushes to the same branch? is it because of the release process, or a way of working, or something different entirely?

@ljharb
Copy link
Member

ljharb commented Apr 7, 2024

Imagine i land two pushes to master/main and the second fails. If the first one is cancelled, how can i quickly know which one introduced the failure?

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 7, 2024

You'd look at the CI for the pull requests you opened to land those changes onto the main branch.

Don't get me wrong I know that's a bit facetious but to me a situation that requires you to push two changes to your default branch which a lot of people would generally like to think is treated as special and ideally always stable in quick succession when you're not sure that either of them will pass your test suite seems like a problem in of its own.

Now compare that to the general cost of running what will (hopefully soon) be 300 CI jobs, some of which are known to be flakey - not only do you have to wait for those to finish, GitHubs concurrency limit per repository is in the 20-60 range, and a small but not zero carbon cost... so to me there's a far bigger advantage in having those jobs automatically cancel when new changes get pushed up.

I'll still have a play to see if there's a way we might be able to not have this apply in some conditions but I'm not hopeful it'll be possible to have the cake and eat it too in this case (at least not with native concurrency)

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 7, 2024

pfft ok all that writing for nothing - you can make cancel-in-progress an expression:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ !contains(github.ref, 'release/')}}

@ljharb
Copy link
Member

ljharb commented Apr 7, 2024

GitHub doesn’t care about its cost, so i shouldn’t either :-) certainly the waiting time can be annoying.

On single-maintainer projects, many changes aren’t landed by pull request.

@ljharb
Copy link
Member

ljharb commented Apr 7, 2024

I’d like it to only apply to PRs, and to not apply to any pushes to any branch, to be clear.

@G-Rath
Copy link
Contributor Author

G-Rath commented Apr 7, 2024

@ljharb should be good to go now

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.github/workflows/node-4+.yml Outdated Show resolved Hide resolved
@G-Rath G-Rath mentioned this pull request Apr 8, 2024
@ljharb ljharb merged commit f77ceb6 into import-js:main Apr 8, 2024
289 checks passed
@G-Rath G-Rath deleted the cancel-in-progress-runs branch April 8, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants