Lint/Prettier + Fixes from #59 (batch cancel + list 100) + Cancel all but latest #35 #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
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
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
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
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."
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
this will allow the cancel all but latest feature to be easier to review
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
changed the title
|
Worth noting I'm fine working with you to part this out to separate PRs if you like, but I needed to have it all in one so I could use it in my workflows for now, and I wasn't sure if that would really help review or not given the commits were all bite-size, but whatever you want with regard to digestion and I can do the necessary git magic :-) |
mikehardy
added a commit
to mikehardy/flutterfire
that referenced
this pull request
Apr 1, 2021
Previously, only workflows older then the current workflow were canceled In very deep queues this means the current instance may not even run until there are future runs enqueued, and those will not run - and thus will not cancel - the current run, leaving the queue in a clogged state Now any time a run instance finally gets off the queue and runs it will check all runs - including runs queued after the current one - and it will clear all non-current runs including itself, hopefully unclogging the queue styfle/cancel-workflow-action#62
Merged
10 tasks
Salakar
pushed a commit
to firebase/flutterfire
that referenced
this pull request
Apr 1, 2021
…5637) Previously, only workflows older then the current workflow were canceled In very deep queues this means the current instance may not even run until there are future runs enqueued, and those will not run - and thus will not cancel - the current run, leaving the queue in a clogged state Now any time a run instance finally gets off the queue and runs it will check all runs - including runs queued after the current one - and it will clear all non-current runs including itself, hopefully unclogging the queue styfle/cancel-workflow-action#62
|
@styfle this is integrated and working correctly in the FlutterFire repo now - https://github.com/FirebaseExtended/flutterfire/runs/2259325892?check_suite_focus=true - that one sees a lot of traffic and uses the "cancel other workflows" style (a different use case than I tested during development) so this is being exercised in production Cheers |
styfle
reviewed
Apr 4, 2021
styfle
reviewed
Apr 4, 2021
This was referenced Apr 5, 2021
|
I'm going to close this one since it was split into multiple PRs, thanks! |
|
Only the first chunk was in separate PRs. If you close this without merging the rest, I've no idea what you're merging as I did not propose the actual useful change as a separate thing yet? |
|
Separate PRs help because I might not accept every change, but maybe some of them are useful like prettier 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hi there!
Apologies for the monster PR but please understand I did my best to make each individual commit easy to review, in repos I maintain if I receive PRs that I really can review commit by commit (and the submitter is willing to rebase / alter individual PRs - I am) it ends up being an easy review. Hopefully you agree...
This PR has a few goals. Here's how it went:
That made me uncomfortable developing anything here, so I did some cleaning first. I adapted the eslint/prettier config from the main GitHub actions repo to the prevailing style here, fixed all the issues that remained, and enforced the typescript compile, prettier format check, and lint run with both a workflow and a commit hook (via husky).
That's the first 7 commits, each small (whitespace commits are the worst, I know) so it's easier to see.
So there's a clean baseline now and it won't require any thought to maintain it.
cancel_newer#59 from @Gislebert about fetching 100 workflow runs at a time, using correct YAML, and canceling workflows in parallelThat's the next 4 commits - each implementing one of the items that seemed like they had positive feedback, all from @Gislebert
I split the workflow run filter into smaller parts in prep for adding the new feature
I add the new
all_but_latestflag from @thomwiggers, use it to determine cancel date range, and filter with it (finally!)I attempted to do this last bit with good comments explaining why each part was happening based on the feedback from that PR
Finally, with the new workflow I added - which you may trigger manually via
workflow_dispatchI tested the whole thing until it worked (using macos which is limited to 5 concurrent runs on a free account, you can easily saturate your queue), and here we are. You can see the test runs here: https://github.com/mikehardy/cancel-workflow-action/actions/workflows/cancel-all-but-latest.ymlFixes #34 - deeply saturated workflow queues should be able to unclog themselves now