Repository navigation
fix: handle non-zipped artifact downloads - #13013
dsanders11 wants to merge 2 commits into
Conversation
github-actions
Bot
added
unmet-requirements
external
|
Thanks for your pull request! Unfortunately, it doesn't meet the minimum requirements for review:
Please update your PR to address the above. Requirements:
This PR will be automatically closed in 7 days if these requirements are not met. |
There was a problem hiding this comment.
Pull request overview
This PR updates gh run download to support downloading GitHub Actions artifacts that are not delivered as zip archives (e.g., uploaded with archive: false), avoiding the current unconditional unzip behavior that fails on non-zip payloads.
Changes:
- Extend the download platform interface to pass artifact name into the download implementation.
- Add a non-zip download path in the HTTP downloader based on the response
Content-Type. - Add test coverage and a fixture for non-zipped artifact downloads.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/run/download/http.go | Adds non-zip handling and updates downloader signature to accept artifact name |
| pkg/cmd/run/download/http_test.go | Updates zip test to set Content-Type and adds a non-zip download test |
| pkg/cmd/run/download/fixtures/mytest.json | Adds a fixture representing non-zipped artifact content |
| pkg/cmd/run/download/download.go | Updates platform interface usage to pass artifact name to Download |
| pkg/cmd/run/download/download_test.go | Updates fake platform to match the new Download signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if resp.Header.Get("Content-Type") != "application/zip" { | ||
| destPath, err := destDir.Join(name) |
There was a problem hiding this comment.
The zip-vs-nonzip decision is based on a strict Content-Type == "application/zip" check. In practice servers often include parameters (e.g. application/zip; charset=binary), omit the header, or use application/octet-stream for zip content, which would route zip downloads into the raw-file path. Parse the media type (e.g. via mime.ParseMediaType) and/or fall back to detecting/attempting zip extraction (and on zip: not a valid zip file, retry as a raw download).
There was a problem hiding this comment.
Considering these are GitHub controlled servers, I think it's reasonable to just use the Content-Type header to make the distinction.
|
Thank you for updating your pull request! It now meets the minimum requirements and has been queued for triage. |
|
Whoops sorry for letting this sit, will try to get to it today! |
|
@williammartin, is there any chance this can be reviewed and shipped please? |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
|
Would it be possible to also add a flag to not unzip the file instead of only relying on the Content-Type header ? To also fix #12435 at the same time |
mark_ready.py moved a Draft to Ready without looking at review threads or the review decision. A PR with an unresolved thread is then returned to Draft by pr-review-merge, and a CHANGES_REQUESTED review was simply ignored, although Ready is meant to say nothing is left to do. - mark_ready reads every page of the PR's review threads (GraphQL, query and answers captured from gh 2.103.3: #201 with none, cli/cli#13013 with one unresolved of three) and refuses Ready while any is unresolved; a lookup that fails or is malformed blocks. - reviewDecision CHANGES_REQUESTED blocks and points at pr-ready, which owns review feedback on a Draft (fix, or dispute with a reply); mark_ready does not triage it. "" (no review required), REVIEW_REQUIRED and APPROVED do not block. Two P4s from the #201 review, both already fail-closed: - a mistyped issue_resolution such as "Partial" is now reported as such by create_pr.py, instead of as a missing "Fixes #N"; - a follow_up_issue_url number longer than 10 digits is a format error; over 4300 digits int() raised and the gate CLI died with a traceback. 10 mutants of the new checks all fail the tests; an unmutated copy passes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Fixes #13012.