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

Optionally cancel all workflows but the latest #35

Merged
merged 4 commits into from Apr 11, 2021

Conversation

thomwiggers
Copy link
Contributor

This helps a bit with the issue described in #34

src/index.ts Outdated Show resolved Hide resolved
@@ -69,6 +79,15 @@ async function main() {
});
console.log(`Cancel run ${id} responded with status ${res.status}`);
}
if (all_but_latest && new Date(current_run.created_at) < cancel_before) {
Copy link
Owner

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?

Copy link
Contributor Author

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...

Copy link
Contributor Author

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 cancel current_run.

Copy link
Owner

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.

Copy link
Contributor Author

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

Copy link
Owner

@styfle styfle left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

I've very quickly rebased, it probably could use another look over the selection logic.

src/index.ts Outdated Show resolved Hide resolved
@styfle
Copy link
Owner

styfle commented Mar 7, 2021

What are your thoughts on #59? They seem to be solving the same use case.

Comment on lines -62 to -64
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'));
Copy link
Contributor

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

@@ -15,6 +15,10 @@ inputs:
access_token:
description: 'Your GitHub Access Token, defaults to: {{ github.token }}'
default: '${{ github.token }}'
required: true
Copy link
Contributor Author

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:

Copy link
Contributor

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?

src/index.ts Outdated
Comment on lines 78 to 85
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}`);
}
Copy link
Contributor

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

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

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

@mikehardy
Copy link
Contributor

(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 )

mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021
This is an adaptation of styfle#35 from @thomwiggers - the logic is entirely
from that PR (thank you!)
mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021
This is an adaptation of styfle#35 from @thomwiggers - the logic is entirely
from that PR (thank you!)

A new workflow adds a 240-second sleep job on macos (limit 5 concurrent)
with manual dispatch available for testing
mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021
This is an adaptation of styfle#35 from @thomwiggers - the logic is entirely
from that PR (thank you!)

A new workflow adds a 240-second sleep job on macos (limit 5 concurrent)
with manual dispatch available for testing
mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021
* chore: add prettier config, format file, add lint workflow

The prettier config was adapted from the official GitHub Actions repo,
bent to fit the prevailing style (where possible) already in the project

The intent is not to be controversial or argue about whitespace, it is just
to have a consistent easy-to-verify style specifically to avoid all arguments
about whitespace. If anything in here is objectionable, just name the setting
to alter and I can edit / re-format / re-push

* chore(lint): add eslint config, check lint in workflow

Similar to the prettier config, this is adapted from the main GitHub Actions repo,
and the intent is not to be controversial it is simply to have any consistent / easy
to check standard. If anything is objectionable, just point out the setting

The config was adapted from the main repo to match what appeared to be prevailing opinion
on this project (semicolons preferred, bracket spacing preferred, etc)

The following commits will fix the small errors that seemed worth fixing

* lint: add explicit return type on main() method

* lint: use for..of vs array.forEach

* lint: workflow_id variable was shadowed, make it unambiguous

* lint: remote ts-ignore via typing workflow_id as number in all places

* chore: add husky and hook build/format/lint checks to pre-commit

This enforces the same checks locally that will execute in CI

With this, everyone should have a clean / consistent dev environment,
and it will be clear to contributors if they submit code that is not valid
typescript

Additionally, after doing the build it adds the dist/index.js output to the
commit list so contributors can't forget to commit it

* fix(ignore_sha): change default from false to 'false'

This was noted by @Gisleburt here styfle#59 (comment)

According to the spec the parameter should be a string, but if you use false without
quotes in YAML it is taken as a boolean so the types are not quite correct

* refactor: extraction of cancel workflow runs method

this is intended to be completely non-functional, nothing should be different
here in how this works from before, it is a pure method extraction

testing this change:
the cancel_self workflow action was updated so it may be run manually, and
the no-longer-required access_token was removed from it

* perf: cancel workflow runs in parallel

This was suggested by @Gisleburt in styfle#59

I quote:

"The next problem we has is that we have multiple jobs starting at once, downloading a list of other jobs and trying to cancel them one at a time. GitHub doesn't wait for the job to be cancelled before returning a 202 response so its possible for two jobs to cancel each other. In order to reduce the chance of this happening we decided to send all of the cancellations in one go, and wait for the 202s in one lump at the end. This change will improve the speed of the task for everyone with more than one workflow to cancel."

* fix: list maximum (100) workflows possible before paging

This was suggested by @Gislebert in styfle#59

the default was 30, this widens the number of workflows we scan to
cancel the most workflows possible, without adding complication by paging

* refactor: break workflow run filter into multiple parts

this will allow the cancel all but latest feature to be easier to review

* feat(all_but_latest): add ability to clear every run but latest

This is an adaptation of styfle#35 from @thomwiggers - the logic is entirely
from that PR (thank you!)

A new workflow adds a 240-second sleep job on macos (limit 5 concurrent)
with manual dispatch available for testing
mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021
This is an adaptation of styfle#35 from @thomwiggers - the logic is entirely
from that PR (thank you!)

A new workflow adds a 240-second sleep job on macos (limit 5 concurrent)
with manual dispatch available for testing
README.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Copy link
Owner

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks, I think this will be a great addition!

@styfle styfle merged commit 1adde81 into styfle:main Apr 11, 2021
@styfle styfle mentioned this pull request Apr 11, 2021
@styfle
Copy link
Owner

styfle commented Apr 11, 2021

I did some cleanup in #67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants