-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Add run download command for fetching workflow artifacts
#3349
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.
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
Conversation
vilmibm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly happy with the usability and direction of this! I otherwise just have questions around testing and code organization.
pkg/cmd/run/view/view.go
Outdated
| "github.com/cli/cli/api" | ||
| "github.com/cli/cli/internal/ghinstance" | ||
| "github.com/cli/cli/internal/ghrepo" | ||
| "github.com/cli/cli/pkg/cmd/run/download" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this dependency be done via a shared package instead of a direct subcommand dependency?
| reg.Register( | ||
| httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/3"), | ||
| httpmock.JSONResponse(shared.SuccessfulRun)) | ||
| reg.Register( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems worth testing the artifact printing in at least one case
- With no arguments in TTY mode, prompt which artifacts to download - Change `--pattern` argument to be just `--name` and only do exact matching - For multi-archive downloads, prefix the destination path with the name of the artifact - Add tests exercising HTTP functionality - Avoid "zipslip" path injection when extracting ZIP files - Add tests for ZIP extraction
78a7a6b to
026dc46
Compare
vilmibm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiniest nitpick about Examples and it needs a fix for the Windows tests but otherwise this is great!
Downloading artifacts per run:
gh run download 123-pflagDownloading artifacts per repository:
gh run download -p name1 -p name2No arguments = error. Should we display some kind of selector in this case? If so, what should the options in the selector be?
Inspirations:
gh release downloadTODO:
Fixes #3321