Optionally cancel all workflows but the latest #35
Conversation
| }); | ||
| console.log(`Cancel run ${id} responded with status ${res.status}`); | ||
| } | ||
| if (all_but_latest && new Date(current_run.created_at) < cancel_before) { |
There was a problem hiding this comment.
What's the purpose of this if statement?
There was a problem hiding this comment.
I can't have a run cancel itself before it has had a chance to cancel others. However, I think https://github.com/styfle/cancel-workflow-action/pull/35/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R69 is a bit sloppy and might break this action if you're not using all_but_latest...
There was a problem hiding this comment.
Ah, no, if you're not all_but_latest you will never cancel current_run.
There was a problem hiding this comment.
Why would this run cancel itself? Shouldn't it always allow itself to run? Otherwise you would have nothing running, everything would cancel everything.
There was a problem hiding this comment.
It should cancel itself if it's not the most recent run
styfle
left a comment
styfle
left a comment
There was a problem hiding this comment.
Thanks for this PR!
I think the premise is good since the readme said "latest" and that wasn't exactly true.
I'm not sure about the implementation...I left a few questions.
thomwiggers
force-pushed
the
all_but_latest
branch
from
085d47c to
e12516c
Compare
|
I've very quickly rebased, it probably could use another look over the selection logic. |
|
What are your thoughts on #59? They seem to be solving the same use case. |
| const branchWorkflows = data.workflow_runs.filter(run => run.head_branch === branch); | ||
| console.log(`Found ${branchWorkflows.length} runs for workflow ${workflow_id} on branch ${branch}`); | ||
| console.log(branchWorkflows.map(run => `- ${run.html_url}`).join('\n')); |
There was a problem hiding this comment.
I think the PR would be easier to review if this change was reverted and the filter below was not done in one big combination, but done separately as before. Then this whole part of the diff would disappear and the filter below would be simpler? That said, it doesn't look incorrect
| access_token: | ||
| description: 'Your GitHub Access Token, defaults to: {{ github.token }}' | ||
| default: '${{ github.token }}' | ||
| required: true |
There was a problem hiding this comment.
this should probably no longer have required:
There was a problem hiding this comment.
yeah it is not actually in 0.8.0? that requirement was removed. Also in the "TIL" department from #59 I learned that YAML won't consider it a string anyway, unless it's in quotes, and the Actions spec says those should be strings. Who knew?
src/index.ts
Outdated
| console.log('Cancelling another run: ', {id, head_sha, status}); | ||
| const res = await octokit.actions.cancelWorkflowRun({ | ||
| owner, | ||
| repo, | ||
| run_id: id | ||
| }); | ||
| console.log(`Cancel run ${id} responded with status ${res.status}`); | ||
| } |
There was a problem hiding this comment.
This PR would be easier to review I think if there was one very focused commit separate from the others that extracted this cancel logic to separate method - then it would not be duplicated below when it is time to cancel the current workflow. Bonus points for ingesting the idea from #59 if the extracted method was not actually a single cancel but took an array of IDs / pushed the Promise result from each cancel on an array / did the Promise.all thing to alleviate some of the "github responds 202 but doesn't actually cancel right away..." issue https://github.com/styfle/cancel-workflow-action/pull/59/files#diff-3d2b59189eeedc2d428ddd632e97658fe310f587f7cb63b01f9b98ffc11c0197R5903-R5914
mikehardy
left a comment
mikehardy
left a comment
There was a problem hiding this comment.
This actually looks good to me though it is not the easiest to review (that's not really a criticism, and my comments are all just thoughts on pushing code around, I'm not sure the interaction between all the flags makes any possible version easy to review!)
It would be great to get something like this in as the FlutterFire repo has the problem of large queue depths and cancels not really helping, where a change like this that allowed older jobs (when finally run) to look forward in the queue and cancel all-but-current + themselves would really free things up firebase/flutterfire#5533
|
(and yes, I know me marking it "approved" means nothing haha - but at least I'm on record, and it's worth noting I use this action everywhere, thanks @styfle ) |
styfle
left a comment
styfle
left a comment
There was a problem hiding this comment.
Thanks, I think this will be a great addition!
|
I did some cleanup in #67 |
Upstream has integrated the "all_but_latest" feature - https://github.com/styfle/cancel-workflow-action/releases/tag/0.9.0 styfle/cancel-workflow-action#35 styfle/cancel-workflow-action#67
Upstream has integrated the "all_but_latest" feature - https://github.com/styfle/cancel-workflow-action/releases/tag/0.9.0 styfle/cancel-workflow-action#35 styfle/cancel-workflow-action#67
3 participants
This helps a bit with the issue described in #34