Add pagination to appropriate listWorkflowRunArtifacts call - take 2 #225
Conversation
robbie-unlikelyai
force-pushed
the
handle-search-artifacts-with-many-artifacts
branch
6 times, most recently
from
e580cfb to
6476da6
Compare
robbie-unlikelyai
force-pushed
the
handle-search-artifacts-with-many-artifacts
branch
from
6476da6 to
e393d88
Compare
robbie-unlikelyai
force-pushed
the
handle-search-artifacts-with-many-artifacts
branch
from
10bcdac to
e96f29b
Compare
|
@dawidd6 / @jamesmortensen - I think this is now good to go. I've added a couple of new test cases to test this properly (and ensure we don't have a repeat of last time!). These tests initially failed (example, showing they would have caught the previous bug) but now pass. |
.github/workflows/upload.yml
Outdated
| strategy: | ||
| matrix: | ||
| # Use a matrix to run this job 40 times with 40 different artifacts. This catches any pagination issues (as GitHub's default page size is 30) | ||
| run-number: [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40 ] |
There was a problem hiding this comment.
That's a lot of artifacts. I get this is done to test the pagination but it adds up quickly to 80 separate jobs when adding upload+download jobs together. Would like to avoid this tbh.
There was a problem hiding this comment.
Yeah, that's definitely a fair concern. I can think of a few options here:
- Just delete the test (i.e. decide the cost of testing this isn't worth the benefit we get from it)
- Leave it as-is (i.e. decide the cost of testing it is worth the benefit we get from it), potentially deleting the 40 download tests and just replacing it with one test to check that we can download one of the artifacts on the second page (leaving us with 41 jobs)
- Have a single job that just runs an upload 40 times - it'd look a bit horrible in the yaml, but would mean we only have 1 job.
Which of those do you think is best @dawidd6 (unless you can think of a better way!)?
There was a problem hiding this comment.
I'm in a favor of the first option. Good you took the time and wrote those tests, we are now sure it works alright. Yet in the long run I don't think we need to run 80 jobs on each push/PR 😄
There was a problem hiding this comment.
Done - hopefully this is now good to go 😄
|
Nice, thanks! Works on master too https://github.com/dawidd6/action-download-artifact/actions/runs/4165023555 |
robbie-unlikelyai
deleted the
handle-search-artifacts-with-many-artifacts
branch
A second attempt at #205 after the first attempt had to be reverted.
More detail in #204.