diff --git a/pkg/cmd/run/download/download.go b/pkg/cmd/run/download/download.go index 15dafa6e8..1c58fd632 100644 --- a/pkg/cmd/run/download/download.go +++ b/pkg/cmd/run/download/download.go @@ -5,25 +5,33 @@ import ( "fmt" "path/filepath" + "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/cli/cli/pkg/set" "github.com/spf13/cobra" ) type DownloadOptions struct { IO *iostreams.IOStreams Platform platform + Prompter prompter + DoPrompt bool RunID string DestinationDir string - FilePatterns []string + Names []string } type platform interface { List(runID string) ([]Artifact, error) Download(url string, dir string) error } +type prompter interface { + Prompt(message string, options []string, result interface{}) error +} func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobra.Command { opts := &DownloadOptions{ @@ -33,22 +41,29 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr cmd := &cobra.Command{ Use: "download []", Short: "Download artifacts generated by a workflow run", - Args: cobra.MaximumNArgs(1), + Long: heredoc.Doc(` + Download artifacts generated by a GitHub Actions workflow run. + + The contents of each artifact will be extracted under separate directories based on + the artifact name. If only a single artifact is specified, it will be extracted into + the current directory. + `), + Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` # Download all artifacts generated by a workflow run $ gh run download # Download a specific artifact within a run - $ gh run download -p + $ gh run download -n # Download specific artifacts across all runs in a repository - $ gh run download -p -p + $ gh run download -n -n `), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { opts.RunID = args[0] - } else if len(opts.FilePatterns) == 0 { - return &cmdutil.FlagError{Err: errors.New("either run ID or `--pattern` is required")} + } else if len(opts.Names) == 0 && opts.IO.CanPrompt() { + opts.DoPrompt = true } // support `-R, --repo` override @@ -64,6 +79,7 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr client: httpClient, repo: baseRepo, } + opts.Prompter = &surveyPrompter{} if runF != nil { return runF(opts) @@ -73,47 +89,79 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr } cmd.Flags().StringVarP(&opts.DestinationDir, "dir", "D", ".", "The directory to download artifacts into") - cmd.Flags().StringArrayVarP(&opts.FilePatterns, "pattern", "p", nil, "Download only artifacts that match a glob pattern") + cmd.Flags().StringArrayVarP(&opts.Names, "name", "n", nil, "Only download artifacts that match any of the given names") return cmd } func runDownload(opts *DownloadOptions) error { opts.IO.StartProgressIndicator() - defer opts.IO.StopProgressIndicator() - artifacts, err := opts.Platform.List(opts.RunID) + opts.IO.StopProgressIndicator() if err != nil { return fmt.Errorf("error fetching artifacts: %w", err) } - // track downloaded artifacts and avoid re-downloading any of the same name - downloaded := map[string]struct{}{} - numArtifacts := 0 - + numValidArtifacts := 0 for _, a := range artifacts { if a.Expired { continue } - numArtifacts++ - if _, found := downloaded[a.Name]; found { + numValidArtifacts++ + } + if numValidArtifacts == 0 { + return errors.New("no valid artifacts found to download") + } + + wantNames := opts.Names + if opts.DoPrompt { + artifactNames := set.NewStringSet() + for _, a := range artifacts { + if !a.Expired { + artifactNames.Add(a.Name) + } + } + options := artifactNames.ToSlice() + if len(options) > 10 { + options = options[:10] + } + err := opts.Prompter.Prompt("Select artifacts to download:", options, &wantNames) + if err != nil { + return err + } + if len(wantNames) == 0 { + return errors.New("no artifacts selected") + } + } + + opts.IO.StartProgressIndicator() + defer opts.IO.StopProgressIndicator() + + // track downloaded artifacts and avoid re-downloading any of the same name + downloaded := set.NewStringSet() + for _, a := range artifacts { + if a.Expired { continue } - if len(opts.FilePatterns) > 0 && !matchAny(opts.FilePatterns, a.Name) { + if downloaded.Contains(a.Name) { continue } - err := opts.Platform.Download(a.DownloadURL, opts.DestinationDir) + if len(wantNames) > 0 && !matchAny(wantNames, a.Name) { + continue + } + destDir := opts.DestinationDir + if len(wantNames) != 1 { + destDir = filepath.Join(destDir, a.Name) + } + err := opts.Platform.Download(a.DownloadURL, destDir) if err != nil { return fmt.Errorf("error downloading %s: %w", a.Name, err) } - downloaded[a.Name] = struct{}{} + downloaded.Add(a.Name) } - if numArtifacts == 0 { - return errors.New("no valid artifacts found to download") - } - if len(downloaded) == 0 { - return errors.New("no artifact matches any of the patterns provided") + if downloaded.Len() == 0 { + return errors.New("no artifact matches any of the names provided") } return nil @@ -121,9 +169,18 @@ func runDownload(opts *DownloadOptions) error { func matchAny(patterns []string, name string) bool { for _, p := range patterns { - if isMatch, err := filepath.Match(p, name); err == nil && isMatch { + if name == p { return true } } return false } + +type surveyPrompter struct{} + +func (sp *surveyPrompter) Prompt(message string, options []string, result interface{}) error { + return prompt.SurveyAskOne(&survey.MultiSelect{ + Message: message, + Options: options, + }, result) +} diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go index 15060f922..23e0d75cc 100644 --- a/pkg/cmd/run/download/download_test.go +++ b/pkg/cmd/run/download/download_test.go @@ -4,6 +4,7 @@ import ( "bytes" "io/ioutil" "net/http" + "path/filepath" "testing" "github.com/cli/cli/internal/ghrepo" @@ -11,6 +12,7 @@ import ( "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -23,10 +25,15 @@ func Test_NewCmdDownload(t *testing.T) { wantErr string }{ { - name: "empty", - args: "", - isTTY: true, - wantErr: "either run ID or `--pattern` is required", + name: "empty", + args: "", + isTTY: true, + want: DownloadOptions{ + RunID: "", + DoPrompt: true, + Names: []string(nil), + DestinationDir: ".", + }, }, { name: "with run ID", @@ -34,7 +41,8 @@ func Test_NewCmdDownload(t *testing.T) { isTTY: true, want: DownloadOptions{ RunID: "2345", - FilePatterns: []string(nil), + DoPrompt: false, + Names: []string(nil), DestinationDir: ".", }, }, @@ -44,17 +52,19 @@ func Test_NewCmdDownload(t *testing.T) { isTTY: true, want: DownloadOptions{ RunID: "2345", - FilePatterns: []string(nil), + DoPrompt: false, + Names: []string(nil), DestinationDir: "tmp/dest", }, }, { - name: "repo level with patterns", - args: "-p one -p two", + name: "repo level with names", + args: "-n one -n two", isTTY: true, want: DownloadOptions{ RunID: "", - FilePatterns: []string{"one", "two"}, + DoPrompt: false, + Names: []string{"one", "two"}, DestinationDir: ".", }, }, @@ -100,47 +110,136 @@ func Test_NewCmdDownload(t *testing.T) { } assert.Equal(t, tt.want.RunID, opts.RunID) - assert.Equal(t, tt.want.FilePatterns, opts.FilePatterns) + assert.Equal(t, tt.want.Names, opts.Names) assert.Equal(t, tt.want.DestinationDir, opts.DestinationDir) + assert.Equal(t, tt.want.DoPrompt, opts.DoPrompt) }) } } func Test_runDownload(t *testing.T) { tests := []struct { - name string - opts DownloadOptions - artifacts []Artifact - wantDownloaded []string - wantErr string + name string + opts DownloadOptions + mockAPI func(*mockPlatform) + mockPrompt func(*mockPrompter) + wantErr string }{ { name: "download non-expired", + opts: DownloadOptions{ + RunID: "2345", + DestinationDir: "./tmp", + Names: []string(nil), + }, + mockAPI: func(p *mockPlatform) { + p.On("List", "2345").Return([]Artifact{ + { + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + { + Name: "expired-artifact", + DownloadURL: "http://download.com/expired.zip", + Expired: true, + }, + { + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + }, nil) + p.On("Download", "http://download.com/artifact1.zip", filepath.FromSlash("tmp/artifact-1")).Return(nil) + p.On("Download", "http://download.com/artifact2.zip", filepath.FromSlash("tmp/artifact-2")).Return(nil) + }, + }, + { + name: "no valid artifacts", opts: DownloadOptions{ RunID: "2345", DestinationDir: ".", - FilePatterns: []string(nil), + Names: []string(nil), }, - artifacts: []Artifact{ - { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact-1.zip", - Expired: false, - }, - { - Name: "expired-artifact", - DownloadURL: "http://download.com/expired.zip", - Expired: true, - }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact-2.zip", - Expired: false, - }, + mockAPI: func(p *mockPlatform) { + p.On("List", "2345").Return([]Artifact{ + { + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: true, + }, + { + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: true, + }, + }, nil) }, - wantDownloaded: []string{ - "http://download.com/artifact-1.zip", - "http://download.com/artifact-2.zip", + wantErr: "no valid artifacts found to download", + }, + { + name: "no matches", + opts: DownloadOptions{ + RunID: "2345", + DestinationDir: ".", + Names: []string{"artifact-3"}, + }, + mockAPI: func(p *mockPlatform) { + p.On("List", "2345").Return([]Artifact{ + { + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + { + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + }, nil) + }, + wantErr: "no artifact matches any of the names provided", + }, + { + name: "prompt to select artifact", + opts: DownloadOptions{ + RunID: "", + DoPrompt: true, + DestinationDir: ".", + Names: []string(nil), + }, + mockAPI: func(p *mockPlatform) { + p.On("List", "").Return([]Artifact{ + { + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + { + Name: "expired-artifact", + DownloadURL: "http://download.com/expired.zip", + Expired: true, + }, + { + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + { + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.also.zip", + Expired: false, + }, + }, nil) + p.On("Download", "http://download.com/artifact2.zip", ".").Return(nil) + }, + mockPrompt: func(p *mockPrompter) { + p.On("Prompt", "Select artifacts to download:", []string{"artifact-1", "artifact-2"}, mock.AnythingOfType("*[]string")). + Run(func(args mock.Arguments) { + result := args.Get(2).(*[]string) + *result = []string{"artifact-2"} + }). + Return(nil) }, }, } @@ -149,8 +248,8 @@ func Test_runDownload(t *testing.T) { opts := &tt.opts io, _, stdout, stderr := iostreams.Test() opts.IO = io - platform := &stubPlatform{listResults: tt.artifacts} - opts.Platform = platform + opts.Platform = newMockPlatform(t, tt.mockAPI) + opts.Prompter = newMockPrompter(t, tt.mockPrompt) err := runDownload(opts) if tt.wantErr != "" { @@ -159,23 +258,55 @@ func Test_runDownload(t *testing.T) { require.NoError(t, err) } - assert.Equal(t, tt.wantDownloaded, platform.downloaded) assert.Equal(t, "", stdout.String()) assert.Equal(t, "", stderr.String()) }) } } -type stubPlatform struct { - listResults []Artifact - downloaded []string +type mockPlatform struct { + mock.Mock } -func (p *stubPlatform) List(runID string) ([]Artifact, error) { - return p.listResults, nil +func newMockPlatform(t *testing.T, config func(*mockPlatform)) *mockPlatform { + m := &mockPlatform{} + m.Test(t) + t.Cleanup(func() { + m.AssertExpectations(t) + }) + if config != nil { + config(m) + } + return m } -func (p *stubPlatform) Download(url string, dir string) error { - p.downloaded = append(p.downloaded, url) - return nil +func (p *mockPlatform) List(runID string) ([]Artifact, error) { + args := p.Called(runID) + return args.Get(0).([]Artifact), args.Error(1) +} + +func (p *mockPlatform) Download(url string, dir string) error { + args := p.Called(url, dir) + return args.Error(0) +} + +type mockPrompter struct { + mock.Mock +} + +func newMockPrompter(t *testing.T, config func(*mockPrompter)) *mockPrompter { + m := &mockPrompter{} + m.Test(t) + t.Cleanup(func() { + m.AssertExpectations(t) + }) + if config != nil { + config(m) + } + return m +} + +func (p *mockPrompter) Prompt(msg string, opts []string, res interface{}) error { + args := p.Called(msg, opts, res) + return args.Error(0) } diff --git a/pkg/cmd/run/download/fixtures/myproject.zip b/pkg/cmd/run/download/fixtures/myproject.zip new file mode 100644 index 000000000..2fdf3f90c Binary files /dev/null and b/pkg/cmd/run/download/fixtures/myproject.zip differ diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index d6413e4ad..52b82d3e9 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -1,6 +1,7 @@ package download import ( + "archive/zip" "encoding/json" "fmt" "io" @@ -62,7 +63,7 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) dec := json.NewDecoder(resp.Body) if err := dec.Decode(&response); err != nil { - return response.Artifacts, fmt.Errorf("error reading JSON: %w", err) + return response.Artifacts, fmt.Errorf("error parsing JSON: %w", err) } return response.Artifacts, nil @@ -73,8 +74,8 @@ func downloadArtifact(httpClient *http.Client, url, destDir string) error { if err != nil { return err } - // The API rejects requesting the "application/octet-stream" type :( - req.Header.Set("Accept", "application/json") + // The server rejects this :( + //req.Header.Set("Accept", "application/zip") resp, err := httpClient.Do(req) if err != nil { @@ -90,19 +91,22 @@ func downloadArtifact(httpClient *http.Client, url, destDir string) error { if err != nil { return fmt.Errorf("error initializing temporary file: %w", err) } - defer tmpfile.Close() + defer func() { + _ = tmpfile.Close() + _ = os.Remove(tmpfile.Name()) + }() - _, err = io.Copy(tmpfile, resp.Body) + size, err := io.Copy(tmpfile, resp.Body) if err != nil { return fmt.Errorf("error writing zip archive: %w", err) } - tmpfile.Close() - if err := extractZip(tmpfile, destDir); err != nil { + zipfile, err := zip.NewReader(tmpfile, size) + if err != nil { return fmt.Errorf("error extracting zip archive: %w", err) } - if err := os.Remove(tmpfile.Name()); err != nil { - return err + if err := extractZip(zipfile, destDir); err != nil { + return fmt.Errorf("error extracting zip archive: %w", err) } return nil diff --git a/pkg/cmd/run/download/http_test.go b/pkg/cmd/run/download/http_test.go new file mode 100644 index 000000000..51a7133de --- /dev/null +++ b/pkg/cmd/run/download/http_test.go @@ -0,0 +1,106 @@ +package download + +import ( + "net/http" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_List(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/123/artifacts"), + httpmock.StringResponse(`{ + "total_count": 2, + "artifacts": [ + {"name": "artifact-1"}, + {"name": "artifact-2"} + ] + }`)) + + api := &apiPlatform{ + client: &http.Client{Transport: reg}, + repo: ghrepo.New("OWNER", "REPO"), + } + artifacts, err := api.List("123") + require.NoError(t, err) + + require.Equal(t, 2, len(artifacts)) + assert.Equal(t, "artifact-1", artifacts[0].Name) + assert.Equal(t, "artifact-2", artifacts[1].Name) +} + +func Test_List_perRepository(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts"), + httpmock.StringResponse(`{}`)) + + api := &apiPlatform{ + client: &http.Client{Transport: reg}, + repo: ghrepo.New("OWNER", "REPO"), + } + _, err := api.List("") + require.NoError(t, err) +} + +func Test_Download(t *testing.T) { + tmpDir := t.TempDir() + destDir := filepath.Join(tmpDir, "artifact") + + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts/12345/zip"), + httpmock.FileResponse("./fixtures/myproject.zip")) + + api := &apiPlatform{ + client: &http.Client{Transport: reg}, + } + err := api.Download("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip", destDir) + require.NoError(t, err) + + var paths []string + parentPrefix := tmpDir + string(filepath.Separator) + err = filepath.Walk(tmpDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if path == tmpDir { + return nil + } + entry := strings.TrimPrefix(path, parentPrefix) + if info.IsDir() { + entry += "/" + } else if info.Mode()&0111 != 0 { + entry += "(X)" + } + paths = append(paths, entry) + return nil + }) + require.NoError(t, err) + + sort.Strings(paths) + assert.Equal(t, []string{ + "artifact/", + "artifact/bin/", + "artifact/bin/myexe", + "artifact/readme.md", + "artifact/src/", + "artifact/src/main.go", + "artifact/src/util.go", + }, paths) +} diff --git a/pkg/cmd/run/download/zip.go b/pkg/cmd/run/download/zip.go index 520a81190..e3959a091 100644 --- a/pkg/cmd/run/download/zip.go +++ b/pkg/cmd/run/download/zip.go @@ -6,29 +6,33 @@ import ( "io" "os" "path/filepath" + "strings" ) -func extractZip(zipfile *os.File, destDir string) error { - zr, err := zip.OpenReader(zipfile.Name()) - if err != nil { - return err - } - defer zr.Close() +const ( + dirMode os.FileMode = 0755 + fileMode os.FileMode = 0644 + execMode os.FileMode = 0755 +) +func extractZip(zr *zip.Reader, destDir string) error { + pathPrefix := filepath.Clean(destDir) + string(filepath.Separator) for _, zf := range zr.File { - fpath := filepath.Join(destDir, zf.Name) + fpath := filepath.Join(destDir, filepath.FromSlash(zf.Name)) + if !strings.HasPrefix(fpath, pathPrefix) { + continue + } if err := extractZipFile(zf, fpath); err != nil { return fmt.Errorf("error extracting %q: %w", zf.Name, err) } } - return nil } func extractZipFile(zf *zip.File, dest string) error { zm := zf.Mode() if zm.IsDir() { - return os.MkdirAll(dest, 0755) + return os.MkdirAll(dest, dirMode) } f, err := zf.Open() @@ -37,15 +41,13 @@ func extractZipFile(zf *zip.File, dest string) error { } defer f.Close() - if err := os.MkdirAll(filepath.Dir(dest), 0755); err != nil { - return err + if dir := filepath.Dir(dest); dir != "." { + if err := os.MkdirAll(dir, dirMode); err != nil { + return err + } } - var mode os.FileMode = 0644 - if isBinary(zm) { - mode = 0755 - } - df, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode) + df, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, getPerm(zm)) if err != nil { return err } @@ -55,6 +57,9 @@ func extractZipFile(zf *zip.File, dest string) error { return err } -func isBinary(m os.FileMode) bool { - return m&0111 != 0 +func getPerm(m os.FileMode) os.FileMode { + if m&0111 == 0 { + return fileMode + } + return execMode }