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.

Sign up for GitHub

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

Jump to bottom

Add pagination to appropriate listWorkflowRunArtifacts call - take 2 #225

Merged
merged 12 commits into from Feb 13, 2023
Merged

Add pagination to appropriate listWorkflowRunArtifacts call - take 2 #225

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1925874
Add testing
robbie-unlikelyai Feb 12, 2023
a715c8c
Merge remote-tracking branch 'upstream/master' into handle-search-art…
robbie-unlikelyai Feb 12, 2023
7a8aa58
Small update
robbie-unlikelyai Feb 12, 2023
4269bb8
Re-add fix
robbie-unlikelyai Feb 12, 2023
e769fd8
Fix CI
robbie-unlikelyai Feb 12, 2023
bdc4de2
Attempt fix
robbie-unlikelyai Feb 12, 2023
dd1c10d
Add some debug logging
robbie-unlikelyai Feb 12, 2023
8ba9d19
Reduce amount of testing so I don't hit API rate limits
robbie-unlikelyai Feb 12, 2023
e393d88
Try fix
robbie-unlikelyai Feb 12, 2023
e57365f
Tidy up
robbie-unlikelyai Feb 12, 2023
e96f29b
Add a couple of extra tests
robbie-unlikelyai Feb 12, 2023
1aaed07
Remove test
robbie-unlikelyai Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 52 additions & 0 deletions .github/workflows/download.yml
View file
Open in desktop
Expand Up @@ -140,3 +140,55 @@ jobs:
dry_run: true
- name: Test
run: test ${{ steps.download.outputs.dry_run }} == false
download-with-check-artifacts:
runs-on: ubuntu-latest
needs: wait
steps:
- name: Checkout
uses: actions/checkout@v3
- name: Download
uses: ./
with:
workflow: upload.yml
name: artifact
path: artifact
check_artifacts: true
- name: Test
run: cat artifact/sha | grep $GITHUB_SHA
download-with-search-artifacts:
runs-on: ubuntu-latest
needs: wait
steps:
- name: Checkout
uses: actions/checkout@v3
- name: Download
uses: ./
with:
workflow: upload.yml
name: artifact
path: artifact
search_artifacts: true
- name: Test
run: cat artifact/sha | grep $GITHUB_SHA
download-many-search-artifacts:
runs-on: ubuntu-latest
needs: wait
strategy:
# 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)
matrix:
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 ]
steps:
- name: Checkout
uses: actions/checkout@v3
- name: Setup outputs
id: setup
run: echo "artifact-name=${{ format('artifact{0}', matrix.run-number) }}" >> $GITHUB_OUTPUT
- name: Download
uses: ./
with:
workflow: upload.yml
name: ${{ steps.setup.outputs.artifact-name }}
path: ${{ steps.setup.outputs.artifact-name }}
search_artifacts: true
- name: Test
run: cat ${{ steps.setup.outputs.artifact-name }}/sha | grep $GITHUB_SHA
19 changes: 19 additions & 0 deletions .github/workflows/upload.yml
View file
Open in desktop
Expand Up @@ -37,3 +37,22 @@ jobs:
with:
name: artifact2
path: artifact2
upload-many:
runs-on: ubuntu-latest
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 ]
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!)?

Copy link
Owner

Choose a reason for hiding this comment

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

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - hopefully this is now good to go 😄

steps:
- name: Setup outputs
id: setup
run: echo "artifact-name=${{ format('artifact{0}', matrix.run-number) }}" >> $GITHUB_OUTPUT
- name: Dump
run: |
mkdir ${{ steps.setup.outputs.artifact-name }}
echo $GITHUB_SHA > ${{ steps.setup.outputs.artifact-name }}/sha
- name: Upload
uses: actions/upload-artifact@v3
with:
name: ${{ steps.setup.outputs.artifact-name }}
path: ${{ steps.setup.outputs.artifact-name }}
6 changes: 3 additions & 3 deletions main.js
View file
Open in desktop
Expand Up @@ -122,16 +122,16 @@ async function main() {
continue
}
if (checkArtifacts || searchArtifacts) {
let artifacts = await client.rest.actions.listWorkflowRunArtifacts({
let artifacts = await client.paginate(client.rest.actions.listWorkflowRunArtifacts, {
owner: owner,
repo: repo,
run_id: run.id,
})
if (artifacts.data.artifacts.length == 0) {
if (!artifacts || artifacts.length == 0) {
continue
}
if (searchArtifacts) {
const artifact = artifacts.data.artifacts.find((artifact) => {
const artifact = artifacts.find((artifact) => {
return artifact.name == name
})
if (!artifact) {
Expand Down