fix(164): Handling of pagination when >5 workflow dispatches happen #165
Conversation
This file contains hidden or 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
…spatches are not missed. Add filter for created timestamp to mitigate the potential for scaling issues (if we follow all the API pages for all workflow dispatches in history, this will take longer and longer. We only actually care about workflow dispatches since this action started).
…is is superceded by filtering by 'created'. Fixup tests
OscarVanL
commented
Apr 8, 2025
Comment on lines
-14
to
-15
| export type WorkflowRunResponse = GetResponseTypeFromEndpointMethod< | ||
| typeof octokit.rest.actions.listWorkflowRuns |
There was a problem hiding this comment.
I expect the typing here can probably be improved, but I'm not really sure what to put here.
OscarVanL
commented
Apr 8, 2025
Comment on lines
-154
to
-155
| const workflowRuns: WorkflowRun[] = response.data.workflow_runs.map( | ||
| workflowRun => ({ |
There was a problem hiding this comment.
This is required because of the poor usage of type annotations for the paginated responses. Maybe there's something we can do better here.
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.
Fixes the handling of paginated responses by using
octokit.paginateto follow all the paginated responses.I expect you will have some comments, I admit I am not experienced with Typescript and have done my best.
To alleviate scaling/performance issues this would introduce on repositories with large numbers of workflow dispatches, the
createdfilter is now used to only include workflow dispatches that happened since thedispatch-workflowaction was started.Justification of
per_pagequery arg removalI expect the original reason for setting the
per_pagevalue of 5 / 10 is because when not using thecreatedtimestamp filter in the API query we'd always get the last 30 workflow dispatches. This is a pretty big API response if you're polling the API. In most use-cases having this many values is redundant as most users will not be initiating many workflow dispatches in a short period.Since making this change to add the
created: >startTimefilter, when the action first starts polling the API, it gets empty responses:And when the API "catches up" to include the workflows we dispatched it then has only those workflows that were dispatched since the action started:
In this case, I think it is appropriate to move to the default page size (30). In most cases, the response will have far fewer values (most of the time, either 0 or 1 values) as the timestamp filter addresses the original need for the small page sizes. A larger page size better handles the use-case I described in my original issue: #164