Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Optionally cancel all workflows but the latest #35
Optionally cancel all workflows but the latest #35
Changes from 2 commits
a865dac
e12516c
a7eadb6
33102da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably no longer have
required:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this
if
statement?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no, if you're not
all_but_latest
you will never cancelcurrent_run
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should cancel itself if it's not the most recent run