artifact(download): skip non-zip files #1874
Conversation
crazy-max
force-pushed
the
test-skip-non-zip
branch
from
17ec324
to
4ec46bb
Compare
|
Added some tests @robherley Sorry for the ping but let me know if this needs anything else for review. |
| core.info(`Artifact download completed successfully.`) | ||
| return {downloadPath, skipped: false} | ||
| } else { | ||
| core.info(`Artifact download skipped.`) |
There was a problem hiding this comment.
If a user requests a specific artifact to download and that artifact can't be downloaded, why should that silently fail?
Shouldn't that be an error?
There was a problem hiding this comment.
I'm reluctant to change the default behavior here as it could be a breaking change for users.
There was a problem hiding this comment.
iiuc the API only expects zip content type for extraction when used with actions/download-artifact@v4:
And it currently breaks workflows when artifacts with other content-type are being downloaded.
I guess we could create an error type if content type does not match so actions/download-artifact can catch it?
There was a problem hiding this comment.
I thought silently skipping unrelated artifacts not uploaded with actions/upload-artifact would be best so it doesn't require any changes in actions/download-artifact.
relates to actions/download-artifact#328 (comment)
At docker we are using the API to upload a build export artifact and we are not using zip format but gzip one:
When using the
actions/download-artifactaction, workflow would fail:As the download API expects a valid zip content type:
toolkit/packages/artifact/src/internal/download/download-artifact.ts
Line 92 in bb2278e
I think we should just skip downloading artifacts that don't have the expected content-type before extracting them.
Can be tested with:
In this workflow we have two files downloaded by "Download artifacts" step. After adding some logging on response headers we can see that the regular artifact uploaded with
actions/upload-artifact@v4haszipascontent-typeheader but one uploaded bydocker/build-push-actionhasapplication/gzip:{ "content-length": "5572", "content-type": "application/gzip", "content-md5": "yPIHPOPuYDEHs/vabwyt6A==", "last-modified": "Mon, 01 Jul 2024 09:22:34 GMT", "accept-ranges": "bytes", "etag": "\"0x8DC99AF5777FC1C\"", "server": "Windows-Azure-Blob/1.0 Microsoft-HTTPAPI/2.0", "x-ms-request-id": "b45769a4-301e-00cf-5498-cb2dab000000", "x-ms-version": "2023-11-03", "x-ms-creation-time": "Mon, 01 Jul 2024 09:22:34 GMT", "x-ms-lease-status": "unlocked", "x-ms-lease-state": "available", "x-ms-blob-type": "BlockBlob", "content-disposition": "attachment; filename=\"docker~test-docker-action~44M6YV.dockerbuild\"", "x-ms-server-encrypted": "true", "access-control-expose-headers": "x-ms-request-id,Server,x-ms-version,Content-Type,Last-Modified,ETag,x-ms-creation-time,Content-MD5,x-ms-lease-status,x-ms-lease-state,x-ms-blob-type,Content-Disposition,x-ms-server-encrypted,Accept-Ranges,Content-Length,Date,Transfer-Encoding", "access-control-allow-origin": "*", "date": "Mon, 01 Jul 2024 09:22:47 GMT" }Step logs:
I think this change should mitigate this issue by making sure we try to extract a valid zip file. And with the deprecation of v3 on December 5, 2024 and brownouts coming in, we are going to have more reports.
cc @thompson-shaun @colinhemmings @tonistiigi