Optionally cancel all workflows but the latest #35

thomwiggers · 2020-09-08T07:57:04Z

This helps a bit with the issue described in #34

src/index.ts

styfle · 2021-02-13T21:41:36Z

src/index.ts

@@ -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) {


What's the purpose of this if statement?

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

Ah, no, if you're not all_but_latest you will never cancel current_run.

Why would this run cancel itself? Shouldn't it always allow itself to run? Otherwise you would have nothing running, everything would cancel everything.

It should cancel itself if it's not the most recent run

styfle

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 · 2021-02-15T10:18:14Z

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

src/index.ts

styfle · 2021-03-07T22:58:58Z

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

mikehardy · 2021-03-31T15:26:12Z

src/index.ts

-      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'));


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

thomwiggers · 2021-03-31T15:31:03Z

action.yml

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


this should probably no longer have required:

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?

mikehardy · 2021-03-31T15:32:25Z

src/index.ts

+        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}`);
+      }


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

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 · 2021-03-31T15:35:38Z

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

@thomwiggers

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

@thomwiggers

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

@thomwiggers

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

@Gisleburt

* 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

@thomwiggers

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

src/index.ts

styfle

Thanks, I think this will be a great addition!

styfle · 2021-04-11T02:17:48Z

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

thomwiggers mentioned this pull request Sep 11, 2020

Queued cancel action can't cancel others #34

Closed

styfle reviewed Feb 13, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

styfle reviewed Feb 13, 2021

View reviewed changes

thomwiggers added 2 commits February 15, 2021 11:13

Cancel all workflows but the latest

a865dac

Update README and fix styfle#34

e12516c

thomwiggers force-pushed the all_but_latest branch from 085d47c to e12516c Compare February 15, 2021 10:17

styfle reviewed Feb 15, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

remove fixme

a7eadb6

thomwiggers mentioned this pull request Mar 8, 2021

Allow cancelling newer builds with cancel_newer #59

Closed

mikehardy reviewed Mar 31, 2021

View reviewed changes

thomwiggers commented Mar 31, 2021

View reviewed changes

mikehardy reviewed Mar 31, 2021

View reviewed changes

mikehardy approved these changes Mar 31, 2021

View reviewed changes

mikehardy added a commit to mikehardy/cancel-workflow-action that referenced this pull request Apr 1, 2021

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

d4f6f2f

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

mikehardy mentioned this pull request Apr 1, 2021

Lint/Prettier + Fixes from #59 (batch cancel + list 100) + Cancel all but latest #35 #62

Closed

styfle reviewed Apr 11, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

styfle reviewed Apr 11, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

Double "L" to single "L"

33102da

styfle approved these changes Apr 11, 2021

View reviewed changes

styfle merged commit 1adde81 into styfle:main Apr 11, 2021

styfle mentioned this pull request Apr 11, 2021

Cleanup all_but_latest #67

Merged

mikehardy mentioned this pull request Apr 11, 2021

chore(workflows): use new cancel-workflow-action release firebase/flutterfire#5723

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally cancel all workflows but the latest #35

Optionally cancel all workflows but the latest #35

thomwiggers commented Sep 8, 2020

styfle Feb 13, 2021

thomwiggers Feb 15, 2021 •

edited

Loading

thomwiggers Feb 15, 2021

styfle Feb 15, 2021

thomwiggers Feb 16, 2021

styfle left a comment

thomwiggers commented Feb 15, 2021

styfle commented Mar 7, 2021

mikehardy Mar 31, 2021

thomwiggers Mar 31, 2021

mikehardy Mar 31, 2021

mikehardy Mar 31, 2021

mikehardy left a comment

mikehardy commented Mar 31, 2021

styfle left a comment

styfle commented Apr 11, 2021

Optionally cancel all workflows but the latest #35

Optionally cancel all workflows but the latest #35

Conversation

thomwiggers commented Sep 8, 2020

Choose a reason for hiding this comment

thomwiggers Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

thomwiggers commented Feb 15, 2021

styfle commented Mar 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Mar 31, 2021

styfle left a comment

Choose a reason for hiding this comment

styfle commented Apr 11, 2021

thomwiggers Feb 15, 2021 •

edited

Loading