From eeca99864087b41fbff6eceed7b6878545722b37 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 8 Sep 2021 17:27:06 -0500 Subject: [PATCH 01/12] binary extension support in gh extension install --- pkg/cmd/extension/command.go | 48 ++++++++++++- pkg/cmd/extension/command_test.go | 53 +++++++++++++- pkg/cmd/extension/http.go | 113 ++++++++++++++++++++++++++++++ pkg/cmd/extension/manager.go | 82 +++++++++++++++++++++- pkg/cmd/extension/manager_test.go | 59 +++++++++++++++- pkg/extensions/extension.go | 6 +- pkg/extensions/manager_mock.go | 93 ++++++++++++++++++------ 7 files changed, 425 insertions(+), 29 deletions(-) create mode 100644 pkg/cmd/extension/http.go diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 3d5215829..f83637681 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -3,10 +3,13 @@ package extension import ( "errors" "fmt" + "net/http" "os" "strings" + "time" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -102,15 +105,38 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { return err } if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { + // TODO i feel like this should check for a gh-foo script return err } + client, err := f.HttpClient() + if err != nil { + return fmt.Errorf("could not make http client: %w", err) + } + client = api.NewCachedClient(client, time.Second*30) + + isBin, err := isBinExtension(client, repo) + if err != nil { + return fmt.Errorf("could not check for binary extension: %w", err) + } + if isBin { + return m.InstallBin(client, repo) + } + + hs, err := hasScript(client, repo) + if err != nil { + return err + } + if !hs { + return errors.New("extension is uninstallable: missing executable") + } + cfg, err := f.Config() if err != nil { return err } protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.Install(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) }, }, func() *cobra.Command { @@ -220,6 +246,26 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, return nil } +func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { + hs, err := hasScript(client, repo) + if err != nil || hs { + return + } + + _, err = fetchLatestRelease(client, repo) + if err != nil { + httpErr, ok := err.(api.HTTPError) + if ok && httpErr.StatusCode == 404 { + err = nil + return + } + return + } + + isBin = true + return +} + func normalizeExtensionSelector(n string) string { if idx := strings.IndexRune(n, '/'); idx >= 0 { n = n[idx+1:] diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 9dc46b3ba..635fcba17 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -3,14 +3,17 @@ package extension import ( "io" "io/ioutil" + "net/http" "os" "strings" "testing" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -25,6 +28,7 @@ func TestNewCmdExtension(t *testing.T) { tests := []struct { name string args []string + httpStubs func(*httpmock.Registry) managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) isTTY bool wantErr bool @@ -33,17 +37,22 @@ func TestNewCmdExtension(t *testing.T) { wantStderr string }{ { - name: "install an extension", + name: "install a git extension", args: []string{"install", "owner/gh-some-ext"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), + httpmock.StringResponse("a script")) + }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallFunc = func(s string, out, errOut io.Writer) error { + em.InstallGitFunc = func(s string, out, errOut io.Writer) error { return nil } return func(t *testing.T) { - installCalls := em.InstallCalls() + installCalls := em.InstallGitCalls() assert.Equal(t, 1, len(installCalls)) assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) listCalls := em.ListCalls() @@ -51,6 +60,33 @@ func TestNewCmdExtension(t *testing.T) { } }, }, + { + name: "install a binary extension", + args: []string{"install", "owner/gh-bin-ext"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-bin-ext/contents/gh-bin-ext"), + httpmock.StatusStringResponse(404, "no")) + reg.Register( + httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), + httpmock.StringResponse("{}")) + }, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func(bool) []extensions.Extension { + return []extensions.Extension{} + } + em.InstallBinFunc = func(_ *http.Client, _ ghrepo.Interface) error { + return nil + } + return func(t *testing.T) { + installCalls := em.InstallBinCalls() + assert.Equal(t, 1, len(installCalls)) + assert.Equal(t, "gh-bin-ext", installCalls[0].Repo.RepoName()) + listCalls := em.ListCalls() + assert.Equal(t, 1, len(listCalls)) + } + }, + }, { name: "install an extension with same name as existing extension", args: []string{"install", "owner/gh-existing-ext"}, @@ -281,12 +317,23 @@ func TestNewCmdExtension(t *testing.T) { assertFunc = tt.managerStubs(em) } + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + if tt.httpStubs != nil { + tt.httpStubs(®) + } + f := cmdutil.Factory{ Config: func() (config.Config, error) { return config.NewBlankConfig(), nil }, IOStreams: ios, ExtensionManager: em, + HttpClient: func() (*http.Client, error) { + return &client, nil + }, } cmd := NewCmdExtension(&f) diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go new file mode 100644 index 000000000..de6f61e4b --- /dev/null +++ b/pkg/cmd/extension/http.go @@ -0,0 +1,113 @@ +package extension + +import ( + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" + "os" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/internal/ghrepo" +) + +func hasScript(httpClient *http.Client, repo ghrepo.Interface) (hs bool, err error) { + path := fmt.Sprintf("repos/%s/%s/contents/%s", + repo.RepoOwner(), repo.RepoName(), repo.RepoName()) + url := ghinstance.RESTPrefix(repo.RepoHost()) + path + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return + } + + resp, err := httpClient.Do(req) + if err != nil { + return + } + defer resp.Body.Close() + + if resp.StatusCode == 404 { + return + } + + if resp.StatusCode > 299 { + err = api.HandleHTTPError(resp) + return + } + + hs = true + return +} + +type releaseAsset struct { + Name string + APIURL string `json:"url"` +} + +type release struct { + Assets []releaseAsset +} + +// downloadAsset downloads a single asset to the given file path. +func downloadAsset(httpClient *http.Client, asset releaseAsset, destPath string) error { + req, err := http.NewRequest("GET", asset.APIURL, nil) + if err != nil { + return err + } + + req.Header.Set("Accept", "application/octet-stream") + + resp, err := httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return api.HandleHTTPError(resp) + } + + f, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0755) + if err != nil { + return err + } + defer f.Close() + + _, err = io.Copy(f, resp.Body) + return err +} + +// fetchLatestRelease finds the latest published release for a repository. +func fetchLatestRelease(httpClient *http.Client, baseRepo ghrepo.Interface) (*release, error) { + path := fmt.Sprintf("repos/%s/%s/releases/latest", baseRepo.RepoOwner(), baseRepo.RepoName()) + url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return nil, api.HandleHTTPError(resp) + } + + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var r release + err = json.Unmarshal(b, &r) + if err != nil { + return nil, err + } + + return &r, nil +} diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index b488704e5..8ffbc422e 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "io/ioutil" + "net/http" "os" "os/exec" "path" @@ -15,9 +16,11 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/findsh" "github.com/cli/safeexec" + "gopkg.in/yaml.v3" ) type Manager struct { @@ -25,6 +28,7 @@ type Manager struct { lookPath func(string) (string, error) findSh func() (string, error) newCommand func(string, ...string) *exec.Cmd + platform func() string } func NewManager() *Manager { @@ -33,6 +37,9 @@ func NewManager() *Manager { lookPath: safeexec.LookPath, findSh: findsh.Find, newCommand: exec.Command, + platform: func() string { + return fmt.Sprintf("%s-%s", runtime.GOOS, runtime.GOARCH) + }, } } @@ -172,7 +179,80 @@ func (m *Manager) InstallLocal(dir string) error { return makeSymlink(dir, targetLink) } -func (m *Manager) Install(cloneURL string, stdout, stderr io.Writer) error { +type BinManifest struct { + Owner string + Name string + Host string + // TODO I may end up not using this; just thinking ahead to local installs + Path string +} + +func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { + var r *release + r, err := fetchLatestRelease(client, repo) + if err != nil { + return err + } + + suffix := m.platform() + var asset *releaseAsset + for _, a := range r.Assets { + if strings.HasSuffix(a.Name, suffix) { + asset = &a + break + } + } + + if asset == nil { + return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R%s/%s -t'Support %s'`", + repo.RepoName(), + suffix, repo.RepoOwner(), repo.RepoName(), suffix) + } + + name := repo.RepoName() + targetDir := filepath.Join(m.installDir(), name) + // TODO clean this up if function errs? + err = os.MkdirAll(targetDir, 0755) + if err != nil { + return fmt.Errorf("failed to create installation directory: %w", err) + } + + binPath := filepath.Join(targetDir, name) + + err = downloadAsset(client, *asset, binPath) + if err != nil { + return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) + } + + manifest := BinManifest{ + Name: name, + Owner: repo.RepoOwner(), + Host: repo.RepoHost(), + Path: binPath, + } + + bs, err := yaml.Marshal(manifest) + if err != nil { + return fmt.Errorf("failed to serialize manifest: %w", err) + } + + manifestPath := filepath.Join(targetDir, "manifest.yml") + + f, err := os.OpenFile(manifestPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) + if err != nil { + return fmt.Errorf("failed to open manifest for writing: %w", err) + } + defer f.Close() + + _, err = f.Write(bs) + if err != nil { + return fmt.Errorf("failed write manifest file: %w", err) + } + + return nil +} + +func (m *Manager) InstallGit(cloneURL string, stdout, stderr io.Writer) error { exe, err := m.lookPath("git") if err != nil { return err diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 2fd458bf1..c03d3fa62 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "io/ioutil" + "net/http" "os" "os/exec" "path/filepath" @@ -11,7 +12,10 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" ) func TestHelperProcess(t *testing.T) { @@ -39,6 +43,9 @@ func newTestManager(dir string) *Manager { cmd.Env = []string{"GH_WANT_HELPER_PROCESS=1"} return cmd }, + platform: func() string { + return "amiga-arm64" + }, } } @@ -190,18 +197,66 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_Install(t *testing.T) { +func TestManager_InstallGit(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} - err := m.Install("https://github.com/owner/gh-some-ext.git", stdout, stderr) + err := m.InstallGit("https://github.com/owner/gh-some-ext.git", stdout, stderr) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) } +func TestManager_InstallBin(t *testing.T) { + repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") + + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-amiga-arm64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "release/cool"), + httpmock.StringResponse("FAKE BINARY")) + + tempDir := t.TempDir() + m := newTestManager(tempDir) + + err := m.InstallBin(&client, repo) + assert.NoError(t, err) + + manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) + assert.NoError(t, err) + + var bm BinManifest + err = yaml.Unmarshal(manifest, &bm) + assert.NoError(t, err) + + assert.Equal(t, BinManifest{ + Name: "gh-bin-ext", + Owner: "owner", + Host: "example.com", + Path: filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext"), + }, bm) + + fakeBin, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext")) + assert.NoError(t, err) + + assert.Equal(t, "FAKE BINARY", string(fakeBin)) +} + func TestManager_Create(t *testing.T) { tempDir := t.TempDir() oldWd, _ := os.Getwd() diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 54379ec7f..6102c1e9a 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -2,6 +2,9 @@ package extensions import ( "io" + "net/http" + + "github.com/cli/cli/v2/internal/ghrepo" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -16,7 +19,8 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - Install(url string, stdout, stderr io.Writer) error + InstallGit(url string, stdout, stderr io.Writer) error + InstallBin(client *http.Client, repo ghrepo.Interface) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index 157262698..97596e70d 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,7 +4,9 @@ package extensions import ( + "github.com/cli/cli/v2/internal/ghrepo" "io" + "net/http" "sync" ) @@ -24,8 +26,11 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallFunc: func(url string, stdout io.Writer, stderr io.Writer) error { -// panic("mock out the Install method") +// InstallBinFunc: func(client *http.Client, repo ghrepo.Interface) error { +// panic("mock out the InstallBin method") +// }, +// InstallGitFunc: func(url string, stdout io.Writer, stderr io.Writer) error { +// panic("mock out the InstallGit method") // }, // InstallLocalFunc: func(dir string) error { // panic("mock out the InstallLocal method") @@ -52,8 +57,11 @@ type ExtensionManagerMock struct { // DispatchFunc mocks the Dispatch method. DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) - // InstallFunc mocks the Install method. - InstallFunc func(url string, stdout io.Writer, stderr io.Writer) error + // InstallBinFunc mocks the InstallBin method. + InstallBinFunc func(client *http.Client, repo ghrepo.Interface) error + + // InstallGitFunc mocks the InstallGit method. + InstallGitFunc func(url string, stdout io.Writer, stderr io.Writer) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -85,8 +93,15 @@ type ExtensionManagerMock struct { // Stderr is the stderr argument value. Stderr io.Writer } - // Install holds details about calls to the Install method. - Install []struct { + // InstallBin holds details about calls to the InstallBin method. + InstallBin []struct { + // Client is the client argument value. + Client *http.Client + // Repo is the repo argument value. + Repo ghrepo.Interface + } + // InstallGit holds details about calls to the InstallGit method. + InstallGit []struct { // URL is the url argument value. URL string // Stdout is the stdout argument value. @@ -123,7 +138,8 @@ type ExtensionManagerMock struct { } lockCreate sync.RWMutex lockDispatch sync.RWMutex - lockInstall sync.RWMutex + lockInstallBin sync.RWMutex + lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex lockList sync.RWMutex lockRemove sync.RWMutex @@ -204,10 +220,45 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { return calls } -// Install calls InstallFunc. -func (mock *ExtensionManagerMock) Install(url string, stdout io.Writer, stderr io.Writer) error { - if mock.InstallFunc == nil { - panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") +// InstallBin calls InstallBinFunc. +func (mock *ExtensionManagerMock) InstallBin(client *http.Client, repo ghrepo.Interface) error { + if mock.InstallBinFunc == nil { + panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") + } + callInfo := struct { + Client *http.Client + Repo ghrepo.Interface + }{ + Client: client, + Repo: repo, + } + mock.lockInstallBin.Lock() + mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) + mock.lockInstallBin.Unlock() + return mock.InstallBinFunc(client, repo) +} + +// InstallBinCalls gets all the calls that were made to InstallBin. +// Check the length with: +// len(mockedExtensionManager.InstallBinCalls()) +func (mock *ExtensionManagerMock) InstallBinCalls() []struct { + Client *http.Client + Repo ghrepo.Interface +} { + var calls []struct { + Client *http.Client + Repo ghrepo.Interface + } + mock.lockInstallBin.RLock() + calls = mock.calls.InstallBin + mock.lockInstallBin.RUnlock() + return calls +} + +// InstallGit calls InstallGitFunc. +func (mock *ExtensionManagerMock) InstallGit(url string, stdout io.Writer, stderr io.Writer) error { + if mock.InstallGitFunc == nil { + panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") } callInfo := struct { URL string @@ -218,16 +269,16 @@ func (mock *ExtensionManagerMock) Install(url string, stdout io.Writer, stderr i Stdout: stdout, Stderr: stderr, } - mock.lockInstall.Lock() - mock.calls.Install = append(mock.calls.Install, callInfo) - mock.lockInstall.Unlock() - return mock.InstallFunc(url, stdout, stderr) + mock.lockInstallGit.Lock() + mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) + mock.lockInstallGit.Unlock() + return mock.InstallGitFunc(url, stdout, stderr) } -// InstallCalls gets all the calls that were made to Install. +// InstallGitCalls gets all the calls that were made to InstallGit. // Check the length with: -// len(mockedExtensionManager.InstallCalls()) -func (mock *ExtensionManagerMock) InstallCalls() []struct { +// len(mockedExtensionManager.InstallGitCalls()) +func (mock *ExtensionManagerMock) InstallGitCalls() []struct { URL string Stdout io.Writer Stderr io.Writer @@ -237,9 +288,9 @@ func (mock *ExtensionManagerMock) InstallCalls() []struct { Stdout io.Writer Stderr io.Writer } - mock.lockInstall.RLock() - calls = mock.calls.Install - mock.lockInstall.RUnlock() + mock.lockInstallGit.RLock() + calls = mock.calls.InstallGit + mock.lockInstallGit.RUnlock() return calls } From b00e8a5681d8934a9408e99c3e8ced693faefe84 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:02:20 -0500 Subject: [PATCH 02/12] more accurately check for binary extension --- pkg/cmd/extension/command.go | 68 +++++++++++++++++++++++++++---- pkg/cmd/extension/command_test.go | 10 +++-- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index f83637681..8153ef5e4 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -247,12 +247,8 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, } func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { - hs, err := hasScript(client, repo) - if err != nil || hs { - return - } - - _, err = fetchLatestRelease(client, repo) + var r *release + r, err = fetchLatestRelease(client, repo) if err != nil { httpErr, ok := err.(api.HTTPError) if ok && httpErr.StatusCode == 404 { @@ -262,7 +258,16 @@ func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err return } - isBin = true + for _, a := range r.Assets { + dists := possibleDists() + for _, d := range dists { + if strings.HasSuffix(a.Name, d) { + isBin = true + break + } + } + } + return } @@ -272,3 +277,52 @@ func normalizeExtensionSelector(n string) string { } return strings.TrimPrefix(n, "gh-") } + +func possibleDists() []string { + return []string{ + "aix-ppc64", + "android-386", + "android-amd64", + "android-arm", + "android-arm64", + "darwin-amd64", + "darwin-arm64", + "dragonfly-amd64", + "freebsd-386", + "freebsd-amd64", + "freebsd-arm", + "freebsd-arm64", + "illumos-amd64", + "ios-amd64", + "ios-arm64", + "js-wasm", + "linux-386", + "linux-amd64", + "linux-arm", + "linux-arm64", + "linux-mips", + "linux-mips64", + "linux-mips64le", + "linux-mipsle", + "linux-ppc64", + "linux-ppc64le", + "linux-riscv64", + "linux-s390x", + "netbsd-386", + "netbsd-amd64", + "netbsd-arm", + "netbsd-arm64", + "openbsd-386", + "openbsd-amd64", + "openbsd-arm", + "openbsd-arm64", + "openbsd-mips64", + "plan9-386", + "plan9-amd64", + "plan9-arm", + "solaris-amd64", + "windows-386", + "windows-amd64", + "windows-arm", + } +} diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 635fcba17..8c54b24da 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -40,6 +40,9 @@ func TestNewCmdExtension(t *testing.T) { name: "install a git extension", args: []string{"install", "owner/gh-some-ext"}, httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), + httpmock.StatusStringResponse(404, "nope")) reg.Register( httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), httpmock.StringResponse("a script")) @@ -64,12 +67,11 @@ func TestNewCmdExtension(t *testing.T) { name: "install a binary extension", args: []string{"install", "owner/gh-bin-ext"}, httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-bin-ext/contents/gh-bin-ext"), - httpmock.StatusStringResponse(404, "no")) reg.Register( httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), - httpmock.StringResponse("{}")) + httpmock.JSONResponse(release{ + Assets: []releaseAsset{ + {Name: "gh-foo-windows-amd64"}}})) }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { From ae38daf08a451bbccb9964fcd8f5a49190cce823 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:05:35 -0500 Subject: [PATCH 03/12] nit --- pkg/cmd/extension/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 8ffbc422e..393e8ef82 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -204,7 +204,7 @@ func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { } if asset == nil { - return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R%s/%s -t'Support %s'`", + return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R %s/%s -t'Support %s'`", repo.RepoName(), suffix, repo.RepoOwner(), repo.RepoName(), suffix) } From f4d97dcedd4b4b4f31ffdf117e357a6fbac5ccae Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:25:26 -0500 Subject: [PATCH 04/12] WIP refactoring --- pkg/cmd/extension/command.go | 22 ++-------------------- pkg/cmd/extension/manager.go | 23 +++++++++++++++++++++++ pkg/cmd/extension/manager_test.go | 4 ++-- pkg/extensions/extension.go | 7 +++++-- 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 8153ef5e4..9ac5e5f21 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -105,38 +105,20 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { return err } if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { - // TODO i feel like this should check for a gh-foo script return err } - client, err := f.HttpClient() if err != nil { return fmt.Errorf("could not make http client: %w", err) } client = api.NewCachedClient(client, time.Second*30) - isBin, err := isBinExtension(client, repo) - if err != nil { - return fmt.Errorf("could not check for binary extension: %w", err) - } - if isBin { - return m.InstallBin(client, repo) - } - - hs, err := hasScript(client, repo) - if err != nil { - return err - } - if !hs { - return errors.New("extension is uninstallable: missing executable") - } - cfg, err := f.Config() if err != nil { return err } - protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + + return m.Install(client, repo, io, cfg) }, }, func() *cobra.Command { diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 393e8ef82..ddfbde3fc 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -19,6 +19,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/findsh" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/safeexec" "gopkg.in/yaml.v3" ) @@ -187,6 +188,28 @@ type BinManifest struct { Path string } +func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostreams.IOStreams, cfg config.Config) error { + isBin, err := isBinExtension(client, repo) + if err != nil { + return fmt.Errorf("could not check for binary extension: %w", err) + } + if isBin { + return m.InstallBin(client, repo) + } + + hs, err := hasScript(client, repo) + if err != nil { + return err + } + if !hs { + // TODO open an issue hint, here? + return errors.New("extension is uninstallable: missing executable") + } + + protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") + return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) +} + func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { var r *release r, err := fetchLatestRelease(client, repo) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index c03d3fa62..80a535c40 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -44,7 +44,7 @@ func newTestManager(dir string) *Manager { return cmd }, platform: func() string { - return "amiga-arm64" + return "windows-amd64" }, } } @@ -222,7 +222,7 @@ func TestManager_InstallBin(t *testing.T) { release{ Assets: []releaseAsset{ { - Name: "gh-bin-ext-amiga-arm64", + Name: "gh-bin-ext-windows-amd64", APIURL: "https://example.com/release/cool", }, }, diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 6102c1e9a..99087fe04 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -4,7 +4,9 @@ import ( "io" "net/http" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/iostreams" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -19,8 +21,9 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - InstallGit(url string, stdout, stderr io.Writer) error - InstallBin(client *http.Client, repo ghrepo.Interface) error + Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error + InstallBin(*http.Client, ghrepo.Interface) error + InstallGit(string, io.Writer, io.Writer) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error From af7805af53384839e3b421d356cf7067d88a09ae Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:46:54 -0500 Subject: [PATCH 05/12] WIP refactoring --- pkg/cmd/extension/command_test.go | 47 +---------- pkg/extensions/manager_mock.go | 135 ++++++++++++++++++++++-------- 2 files changed, 103 insertions(+), 79 deletions(-) diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 8c54b24da..9b363fbb7 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -28,7 +28,6 @@ func TestNewCmdExtension(t *testing.T) { tests := []struct { name string args []string - httpStubs func(*httpmock.Registry) managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) isTTY bool wantErr bool @@ -37,53 +36,19 @@ func TestNewCmdExtension(t *testing.T) { wantStderr string }{ { - name: "install a git extension", + name: "install an extension", args: []string{"install", "owner/gh-some-ext"}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), - httpmock.StatusStringResponse(404, "nope")) - reg.Register( - httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), - httpmock.StringResponse("a script")) - }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallGitFunc = func(s string, out, errOut io.Writer) error { + em.InstallFunc = func(_ *http.Client, _ ghrepo.Interface, _ *iostreams.IOStreams, _ config.Config) error { return nil } return func(t *testing.T) { - installCalls := em.InstallGitCalls() + installCalls := em.InstallCalls() assert.Equal(t, 1, len(installCalls)) - assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) - listCalls := em.ListCalls() - assert.Equal(t, 1, len(listCalls)) - } - }, - }, - { - name: "install a binary extension", - args: []string{"install", "owner/gh-bin-ext"}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), - httpmock.JSONResponse(release{ - Assets: []releaseAsset{ - {Name: "gh-foo-windows-amd64"}}})) - }, - managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { - em.ListFunc = func(bool) []extensions.Extension { - return []extensions.Extension{} - } - em.InstallBinFunc = func(_ *http.Client, _ ghrepo.Interface) error { - return nil - } - return func(t *testing.T) { - installCalls := em.InstallBinCalls() - assert.Equal(t, 1, len(installCalls)) - assert.Equal(t, "gh-bin-ext", installCalls[0].Repo.RepoName()) + assert.Equal(t, "gh-some-ext", installCalls[0].InterfaceMoqParam.RepoName()) listCalls := em.ListCalls() assert.Equal(t, 1, len(listCalls)) } @@ -323,10 +288,6 @@ func TestNewCmdExtension(t *testing.T) { defer reg.Verify(t) client := http.Client{Transport: ®} - if tt.httpStubs != nil { - tt.httpStubs(®) - } - f := cmdutil.Factory{ Config: func() (config.Config, error) { return config.NewBlankConfig(), nil diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index 97596e70d..b8d18dc6d 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,7 +4,9 @@ package extensions import ( + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/iostreams" "io" "net/http" "sync" @@ -26,10 +28,13 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallBinFunc: func(client *http.Client, repo ghrepo.Interface) error { +// InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +// panic("mock out the Install method") +// }, +// InstallBinFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface) error { // panic("mock out the InstallBin method") // }, -// InstallGitFunc: func(url string, stdout io.Writer, stderr io.Writer) error { +// InstallGitFunc: func(s string, writer1 io.Writer, writer2 io.Writer) error { // panic("mock out the InstallGit method") // }, // InstallLocalFunc: func(dir string) error { @@ -57,11 +62,14 @@ type ExtensionManagerMock struct { // DispatchFunc mocks the Dispatch method. DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) + // InstallFunc mocks the Install method. + InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error + // InstallBinFunc mocks the InstallBin method. - InstallBinFunc func(client *http.Client, repo ghrepo.Interface) error + InstallBinFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface) error // InstallGitFunc mocks the InstallGit method. - InstallGitFunc func(url string, stdout io.Writer, stderr io.Writer) error + InstallGitFunc func(s string, writer1 io.Writer, writer2 io.Writer) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -93,21 +101,32 @@ type ExtensionManagerMock struct { // Stderr is the stderr argument value. Stderr io.Writer } + // Install holds details about calls to the Install method. + Install []struct { + // Client is the client argument value. + Client *http.Client + // InterfaceMoqParam is the interfaceMoqParam argument value. + InterfaceMoqParam ghrepo.Interface + // IOStreams is the iOStreams argument value. + IOStreams *iostreams.IOStreams + // ConfigMoqParam is the configMoqParam argument value. + ConfigMoqParam config.Config + } // InstallBin holds details about calls to the InstallBin method. InstallBin []struct { // Client is the client argument value. Client *http.Client - // Repo is the repo argument value. - Repo ghrepo.Interface + // InterfaceMoqParam is the interfaceMoqParam argument value. + InterfaceMoqParam ghrepo.Interface } // InstallGit holds details about calls to the InstallGit method. InstallGit []struct { - // URL is the url argument value. - URL string - // Stdout is the stdout argument value. - Stdout io.Writer - // Stderr is the stderr argument value. - Stderr io.Writer + // S is the s argument value. + S string + // Writer1 is the writer1 argument value. + Writer1 io.Writer + // Writer2 is the writer2 argument value. + Writer2 io.Writer } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { @@ -138,6 +157,7 @@ type ExtensionManagerMock struct { } lockCreate sync.RWMutex lockDispatch sync.RWMutex + lockInstall sync.RWMutex lockInstallBin sync.RWMutex lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex @@ -220,34 +240,77 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { return calls } +// Install calls InstallFunc. +func (mock *ExtensionManagerMock) Install(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { + if mock.InstallFunc == nil { + panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") + } + callInfo := struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config + }{ + Client: client, + InterfaceMoqParam: interfaceMoqParam, + IOStreams: iOStreams, + ConfigMoqParam: configMoqParam, + } + mock.lockInstall.Lock() + mock.calls.Install = append(mock.calls.Install, callInfo) + mock.lockInstall.Unlock() + return mock.InstallFunc(client, interfaceMoqParam, iOStreams, configMoqParam) +} + +// InstallCalls gets all the calls that were made to Install. +// Check the length with: +// len(mockedExtensionManager.InstallCalls()) +func (mock *ExtensionManagerMock) InstallCalls() []struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config +} { + var calls []struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config + } + mock.lockInstall.RLock() + calls = mock.calls.Install + mock.lockInstall.RUnlock() + return calls +} + // InstallBin calls InstallBinFunc. -func (mock *ExtensionManagerMock) InstallBin(client *http.Client, repo ghrepo.Interface) error { +func (mock *ExtensionManagerMock) InstallBin(client *http.Client, interfaceMoqParam ghrepo.Interface) error { if mock.InstallBinFunc == nil { panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") } callInfo := struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface }{ - Client: client, - Repo: repo, + Client: client, + InterfaceMoqParam: interfaceMoqParam, } mock.lockInstallBin.Lock() mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) mock.lockInstallBin.Unlock() - return mock.InstallBinFunc(client, repo) + return mock.InstallBinFunc(client, interfaceMoqParam) } // InstallBinCalls gets all the calls that were made to InstallBin. // Check the length with: // len(mockedExtensionManager.InstallBinCalls()) func (mock *ExtensionManagerMock) InstallBinCalls() []struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface } { var calls []struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface } mock.lockInstallBin.RLock() calls = mock.calls.InstallBin @@ -256,37 +319,37 @@ func (mock *ExtensionManagerMock) InstallBinCalls() []struct { } // InstallGit calls InstallGitFunc. -func (mock *ExtensionManagerMock) InstallGit(url string, stdout io.Writer, stderr io.Writer) error { +func (mock *ExtensionManagerMock) InstallGit(s string, writer1 io.Writer, writer2 io.Writer) error { if mock.InstallGitFunc == nil { panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") } callInfo := struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer }{ - URL: url, - Stdout: stdout, - Stderr: stderr, + S: s, + Writer1: writer1, + Writer2: writer2, } mock.lockInstallGit.Lock() mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) mock.lockInstallGit.Unlock() - return mock.InstallGitFunc(url, stdout, stderr) + return mock.InstallGitFunc(s, writer1, writer2) } // InstallGitCalls gets all the calls that were made to InstallGit. // Check the length with: // len(mockedExtensionManager.InstallGitCalls()) func (mock *ExtensionManagerMock) InstallGitCalls() []struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer } { var calls []struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer } mock.lockInstallGit.RLock() calls = mock.calls.InstallGit From f5d269ebad0be6fcf42a0e76a87cc887f3ab37bd Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:02:34 -0500 Subject: [PATCH 06/12] WIP refactoring --- pkg/cmd/extension/manager.go | 1 + pkg/cmd/extension/manager_test.go | 48 +++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index ddfbde3fc..75cb4cfd9 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -185,6 +185,7 @@ type BinManifest struct { Name string Host string // TODO I may end up not using this; just thinking ahead to local installs + // TODO track version Path string } diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 80a535c40..f0a930dcd 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -12,8 +12,10 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" "gopkg.in/yaml.v3" ) @@ -197,25 +199,57 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_InstallGit(t *testing.T) { +func TestManager_Install_git(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) - stdout := &bytes.Buffer{} - stderr := &bytes.Buffer{} - err := m.InstallGit("https://github.com/owner/gh-some-ext.git", stdout, stderr) + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "not-a-binary", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), + httpmock.StringResponse("script")) + + io, _, stdout, stderr := iostreams.Test() + + repo := ghrepo.New("owner", "gh-some-ext") + + err := m.Install(&client, repo, io, config.NewBlankConfig()) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) } -func TestManager_InstallBin(t *testing.T) { +func TestManager_Install_binary(t *testing.T) { repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") reg := httpmock.Registry{} defer reg.Verify(t) client := http.Client{Transport: ®} + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-windows-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) reg.Register( httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), httpmock.JSONResponse( @@ -234,7 +268,9 @@ func TestManager_InstallBin(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) - err := m.InstallBin(&client, repo) + io, _, _, _ := iostreams.Test() + + err := m.Install(&client, repo, io, config.NewBlankConfig()) assert.NoError(t, err) manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) From 0e2861a507cc079bed0f5883bd98f38f9cacee92 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:05:19 -0500 Subject: [PATCH 07/12] WIP refactoring --- pkg/cmd/extension/manager.go | 8 +-- pkg/extensions/extension.go | 2 - pkg/extensions/manager_mock.go | 104 --------------------------------- 3 files changed, 4 insertions(+), 110 deletions(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 75cb4cfd9..db6ec76ba 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -195,7 +195,7 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre return fmt.Errorf("could not check for binary extension: %w", err) } if isBin { - return m.InstallBin(client, repo) + return m.installBin(client, repo) } hs, err := hasScript(client, repo) @@ -208,10 +208,10 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre } protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) } -func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { +func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { var r *release r, err := fetchLatestRelease(client, repo) if err != nil { @@ -276,7 +276,7 @@ func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { return nil } -func (m *Manager) InstallGit(cloneURL string, stdout, stderr io.Writer) error { +func (m *Manager) installGit(cloneURL string, stdout, stderr io.Writer) error { exe, err := m.lookPath("git") if err != nil { return err diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 99087fe04..16a3c7494 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -22,8 +22,6 @@ type Extension interface { type ExtensionManager interface { List(includeMetadata bool) []Extension Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error - InstallBin(*http.Client, ghrepo.Interface) error - InstallGit(string, io.Writer, io.Writer) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index b8d18dc6d..ce7a71ce8 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -31,12 +31,6 @@ var _ ExtensionManager = &ExtensionManagerMock{} // InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { // panic("mock out the Install method") // }, -// InstallBinFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface) error { -// panic("mock out the InstallBin method") -// }, -// InstallGitFunc: func(s string, writer1 io.Writer, writer2 io.Writer) error { -// panic("mock out the InstallGit method") -// }, // InstallLocalFunc: func(dir string) error { // panic("mock out the InstallLocal method") // }, @@ -65,12 +59,6 @@ type ExtensionManagerMock struct { // InstallFunc mocks the Install method. InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error - // InstallBinFunc mocks the InstallBin method. - InstallBinFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface) error - - // InstallGitFunc mocks the InstallGit method. - InstallGitFunc func(s string, writer1 io.Writer, writer2 io.Writer) error - // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -112,22 +100,6 @@ type ExtensionManagerMock struct { // ConfigMoqParam is the configMoqParam argument value. ConfigMoqParam config.Config } - // InstallBin holds details about calls to the InstallBin method. - InstallBin []struct { - // Client is the client argument value. - Client *http.Client - // InterfaceMoqParam is the interfaceMoqParam argument value. - InterfaceMoqParam ghrepo.Interface - } - // InstallGit holds details about calls to the InstallGit method. - InstallGit []struct { - // S is the s argument value. - S string - // Writer1 is the writer1 argument value. - Writer1 io.Writer - // Writer2 is the writer2 argument value. - Writer2 io.Writer - } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { // Dir is the dir argument value. @@ -158,8 +130,6 @@ type ExtensionManagerMock struct { lockCreate sync.RWMutex lockDispatch sync.RWMutex lockInstall sync.RWMutex - lockInstallBin sync.RWMutex - lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex lockList sync.RWMutex lockRemove sync.RWMutex @@ -283,80 +253,6 @@ func (mock *ExtensionManagerMock) InstallCalls() []struct { return calls } -// InstallBin calls InstallBinFunc. -func (mock *ExtensionManagerMock) InstallBin(client *http.Client, interfaceMoqParam ghrepo.Interface) error { - if mock.InstallBinFunc == nil { - panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") - } - callInfo := struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface - }{ - Client: client, - InterfaceMoqParam: interfaceMoqParam, - } - mock.lockInstallBin.Lock() - mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) - mock.lockInstallBin.Unlock() - return mock.InstallBinFunc(client, interfaceMoqParam) -} - -// InstallBinCalls gets all the calls that were made to InstallBin. -// Check the length with: -// len(mockedExtensionManager.InstallBinCalls()) -func (mock *ExtensionManagerMock) InstallBinCalls() []struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface -} { - var calls []struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface - } - mock.lockInstallBin.RLock() - calls = mock.calls.InstallBin - mock.lockInstallBin.RUnlock() - return calls -} - -// InstallGit calls InstallGitFunc. -func (mock *ExtensionManagerMock) InstallGit(s string, writer1 io.Writer, writer2 io.Writer) error { - if mock.InstallGitFunc == nil { - panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") - } - callInfo := struct { - S string - Writer1 io.Writer - Writer2 io.Writer - }{ - S: s, - Writer1: writer1, - Writer2: writer2, - } - mock.lockInstallGit.Lock() - mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) - mock.lockInstallGit.Unlock() - return mock.InstallGitFunc(s, writer1, writer2) -} - -// InstallGitCalls gets all the calls that were made to InstallGit. -// Check the length with: -// len(mockedExtensionManager.InstallGitCalls()) -func (mock *ExtensionManagerMock) InstallGitCalls() []struct { - S string - Writer1 io.Writer - Writer2 io.Writer -} { - var calls []struct { - S string - Writer1 io.Writer - Writer2 io.Writer - } - mock.lockInstallGit.RLock() - calls = mock.calls.InstallGit - mock.lockInstallGit.RUnlock() - return calls -} - // InstallLocal calls InstallLocalFunc. func (mock *ExtensionManagerMock) InstallLocal(dir string) error { if mock.InstallLocalFunc == nil { From e85b0480e90820a52dbbf8748602d67eced01524 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:10:18 -0500 Subject: [PATCH 08/12] track installed tag name --- pkg/cmd/extension/http.go | 1 + pkg/cmd/extension/manager.go | 7 ++++--- pkg/cmd/extension/manager_test.go | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go index de6f61e4b..6f93f2303 100644 --- a/pkg/cmd/extension/http.go +++ b/pkg/cmd/extension/http.go @@ -47,6 +47,7 @@ type releaseAsset struct { } type release struct { + Tag string `json:"tag_name"` Assets []releaseAsset } diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index db6ec76ba..05e048588 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -180,12 +180,12 @@ func (m *Manager) InstallLocal(dir string) error { return makeSymlink(dir, targetLink) } -type BinManifest struct { +type binManifest struct { Owner string Name string Host string + Tag string // TODO I may end up not using this; just thinking ahead to local installs - // TODO track version Path string } @@ -248,11 +248,12 @@ func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } - manifest := BinManifest{ + manifest := binManifest{ Name: name, Owner: repo.RepoOwner(), Host: repo.RepoHost(), Path: binPath, + Tag: r.Tag, } bs, err := yaml.Marshal(manifest) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index f0a930dcd..8c4ca604e 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -254,6 +254,7 @@ func TestManager_Install_binary(t *testing.T) { httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), httpmock.JSONResponse( release{ + Tag: "v1.0.1", Assets: []releaseAsset{ { Name: "gh-bin-ext-windows-amd64", @@ -276,14 +277,15 @@ func TestManager_Install_binary(t *testing.T) { manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) assert.NoError(t, err) - var bm BinManifest + var bm binManifest err = yaml.Unmarshal(manifest, &bm) assert.NoError(t, err) - assert.Equal(t, BinManifest{ + assert.Equal(t, binManifest{ Name: "gh-bin-ext", Owner: "owner", Host: "example.com", + Tag: "v1.0.1", Path: filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext"), }, bm) From 1f3b872859d33a5e5d1a4445c66f10aa6899b88f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:17:18 -0500 Subject: [PATCH 09/12] test for unsupported platform --- pkg/cmd/extension/manager_test.go | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 8c4ca604e..3528d9920 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -232,6 +232,50 @@ func TestManager_Install_git(t *testing.T) { assert.Equal(t, "", stderr.String()) } +func TestManager_Install_binary_unsupported(t *testing.T) { + repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") + + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-linux-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Tag: "v1.0.1", + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-linux-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + + tempDir := t.TempDir() + m := newTestManager(tempDir) + + io, _, _, _ := iostreams.Test() + + err := m.Install(&client, repo, io, config.NewBlankConfig()) + assert.Error(t, err) + + errText := "gh-bin-ext unsupported for windows-amd64. Open an issue: `gh issue create -R owner/gh-bin-ext -t'Support windows-amd64'`" + + assert.Equal(t, errText, err.Error()) +} + func TestManager_Install_binary(t *testing.T) { repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") From 514d4d992ce4900f18f286b241d788a01c721d0e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Sep 2021 15:55:31 -0500 Subject: [PATCH 10/12] refactor dependencies of ext manager --- pkg/cmd/extension/command.go | 82 +---------------------- pkg/cmd/extension/command_test.go | 2 +- pkg/cmd/extension/manager.go | 106 +++++++++++++++++++++++++++--- pkg/cmd/extension/manager_test.go | 48 +++++++------- pkg/cmd/factory/default.go | 20 +++++- pkg/extensions/extension.go | 5 +- pkg/extensions/manager_mock.go | 29 ++------ 7 files changed, 148 insertions(+), 144 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 9ac5e5f21..03466b84d 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -3,7 +3,6 @@ package extension import ( "errors" "fmt" - "net/http" "os" "strings" "time" @@ -113,12 +112,7 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { } client = api.NewCachedClient(client, time.Second*30) - cfg, err := f.Config() - if err != nil { - return err - } - - return m.Install(client, repo, io, cfg) + return m.Install(repo) }, }, func() *cobra.Command { @@ -228,83 +222,9 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, return nil } -func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { - var r *release - r, err = fetchLatestRelease(client, repo) - if err != nil { - httpErr, ok := err.(api.HTTPError) - if ok && httpErr.StatusCode == 404 { - err = nil - return - } - return - } - - for _, a := range r.Assets { - dists := possibleDists() - for _, d := range dists { - if strings.HasSuffix(a.Name, d) { - isBin = true - break - } - } - } - - return -} - func normalizeExtensionSelector(n string) string { if idx := strings.IndexRune(n, '/'); idx >= 0 { n = n[idx+1:] } return strings.TrimPrefix(n, "gh-") } - -func possibleDists() []string { - return []string{ - "aix-ppc64", - "android-386", - "android-amd64", - "android-arm", - "android-arm64", - "darwin-amd64", - "darwin-arm64", - "dragonfly-amd64", - "freebsd-386", - "freebsd-amd64", - "freebsd-arm", - "freebsd-arm64", - "illumos-amd64", - "ios-amd64", - "ios-arm64", - "js-wasm", - "linux-386", - "linux-amd64", - "linux-arm", - "linux-arm64", - "linux-mips", - "linux-mips64", - "linux-mips64le", - "linux-mipsle", - "linux-ppc64", - "linux-ppc64le", - "linux-riscv64", - "linux-s390x", - "netbsd-386", - "netbsd-amd64", - "netbsd-arm", - "netbsd-arm64", - "openbsd-386", - "openbsd-amd64", - "openbsd-arm", - "openbsd-arm64", - "openbsd-mips64", - "plan9-386", - "plan9-amd64", - "plan9-arm", - "solaris-amd64", - "windows-386", - "windows-amd64", - "windows-arm", - } -} diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 9b363fbb7..fac5d0e9a 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -42,7 +42,7 @@ func TestNewCmdExtension(t *testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallFunc = func(_ *http.Client, _ ghrepo.Interface, _ *iostreams.IOStreams, _ config.Config) error { + em.InstallFunc = func(_ ghrepo.Interface) error { return nil } return func(t *testing.T) { diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 05e048588..7e1f403e2 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -15,6 +15,7 @@ import ( "strings" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" @@ -30,9 +31,12 @@ type Manager struct { findSh func() (string, error) newCommand func(string, ...string) *exec.Cmd platform func() string + client *http.Client + config config.Config + io *iostreams.IOStreams } -func NewManager() *Manager { +func NewManager(io *iostreams.IOStreams) *Manager { return &Manager{ dataDir: config.DataDir, lookPath: safeexec.LookPath, @@ -44,6 +48,14 @@ func NewManager() *Manager { } } +func (m *Manager) SetConfig(cfg config.Config) { + m.config = cfg +} + +func (m *Manager) SetClient(client *http.Client) { + m.client = client +} + func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Writer) (bool, error) { if len(args) == 0 { return false, errors.New("too few arguments in list") @@ -189,16 +201,16 @@ type binManifest struct { Path string } -func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostreams.IOStreams, cfg config.Config) error { - isBin, err := isBinExtension(client, repo) +func (m *Manager) Install(repo ghrepo.Interface) error { + isBin, err := isBinExtension(m.client, repo) if err != nil { return fmt.Errorf("could not check for binary extension: %w", err) } if isBin { - return m.installBin(client, repo) + return m.installBin(repo) } - hs, err := hasScript(client, repo) + hs, err := hasScript(m.client, repo) if err != nil { return err } @@ -207,13 +219,13 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre return errors.New("extension is uninstallable: missing executable") } - protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + protocol, _ := m.config.Get(repo.RepoHost(), "git_protocol") + return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), m.io.Out, m.io.ErrOut) } -func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { +func (m *Manager) installBin(repo ghrepo.Interface) error { var r *release - r, err := fetchLatestRelease(client, repo) + r, err := fetchLatestRelease(m.client, repo) if err != nil { return err } @@ -243,7 +255,7 @@ func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { binPath := filepath.Join(targetDir, name) - err = downloadAsset(client, *asset, binPath) + err = downloadAsset(m.client, *asset, binPath) if err != nil { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } @@ -451,3 +463,77 @@ func readPathFromFile(path string) (string, error) { n, err := f.Read(b) return strings.TrimSpace(string(b[:n])), err } + +func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { + var r *release + r, err = fetchLatestRelease(client, repo) + if err != nil { + httpErr, ok := err.(api.HTTPError) + if ok && httpErr.StatusCode == 404 { + err = nil + return + } + return + } + + for _, a := range r.Assets { + dists := possibleDists() + for _, d := range dists { + if strings.HasSuffix(a.Name, d) { + isBin = true + break + } + } + } + + return +} + +func possibleDists() []string { + return []string{ + "aix-ppc64", + "android-386", + "android-amd64", + "android-arm", + "android-arm64", + "darwin-amd64", + "darwin-arm64", + "dragonfly-amd64", + "freebsd-386", + "freebsd-amd64", + "freebsd-arm", + "freebsd-arm64", + "illumos-amd64", + "ios-amd64", + "ios-arm64", + "js-wasm", + "linux-386", + "linux-amd64", + "linux-arm", + "linux-arm64", + "linux-mips", + "linux-mips64", + "linux-mips64le", + "linux-mipsle", + "linux-ppc64", + "linux-ppc64le", + "linux-riscv64", + "linux-s390x", + "netbsd-386", + "netbsd-amd64", + "netbsd-arm", + "netbsd-arm64", + "openbsd-386", + "openbsd-amd64", + "openbsd-arm", + "openbsd-arm64", + "openbsd-mips64", + "plan9-386", + "plan9-amd64", + "plan9-arm", + "solaris-amd64", + "windows-386", + "windows-amd64", + "windows-arm", + } +} diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 3528d9920..d78f4c2e6 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -34,7 +34,7 @@ func TestHelperProcess(t *testing.T) { os.Exit(0) } -func newTestManager(dir string) *Manager { +func newTestManager(dir string, client *http.Client, io *iostreams.IOStreams) *Manager { return &Manager{ dataDir: func() string { return dir }, lookPath: func(exe string) (string, error) { return exe, nil }, @@ -45,6 +45,9 @@ func newTestManager(dir string) *Manager { cmd.Env = []string{"GH_WANT_HELPER_PROCESS=1"} return cmd }, + config: config.NewBlankConfig(), + io: io, + client: client, platform: func() string { return "windows-amd64" }, @@ -56,7 +59,7 @@ func TestManager_List(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) exts := m.List(false) assert.Equal(t, 2, len(exts)) assert.Equal(t, "hello", exts[0].Name()) @@ -68,7 +71,7 @@ func TestManager_Dispatch(t *testing.T) { extPath := filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello") assert.NoError(t, stubExtension(extPath)) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -89,7 +92,7 @@ func TestManager_Remove(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) err := m.Remove("hello") assert.NoError(t, err) @@ -105,7 +108,7 @@ func TestManager_Upgrade_AllExtensions(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -130,7 +133,7 @@ func TestManager_Upgrade_RemoteExtension(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -150,7 +153,7 @@ func TestManager_Upgrade_LocalExtension(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -167,7 +170,7 @@ func TestManager_Upgrade_Force(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -189,7 +192,7 @@ func TestManager_Upgrade_Force(t *testing.T) { func TestManager_Upgrade_NoExtensions(t *testing.T) { tempDir := t.TempDir() - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -201,12 +204,15 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { func TestManager_Install_git(t *testing.T) { tempDir := t.TempDir() - m := newTestManager(tempDir) reg := httpmock.Registry{} defer reg.Verify(t) client := http.Client{Transport: ®} + io, _, stdout, stderr := iostreams.Test() + + m := newTestManager(tempDir, &client, io) + reg.Register( httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), httpmock.JSONResponse( @@ -222,11 +228,9 @@ func TestManager_Install_git(t *testing.T) { httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), httpmock.StringResponse("script")) - io, _, stdout, stderr := iostreams.Test() - repo := ghrepo.New("owner", "gh-some-ext") - err := m.Install(&client, repo, io, config.NewBlankConfig()) + err := m.Install(repo) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) @@ -263,12 +267,12 @@ func TestManager_Install_binary_unsupported(t *testing.T) { }, })) - tempDir := t.TempDir() - m := newTestManager(tempDir) - io, _, _, _ := iostreams.Test() + tempDir := t.TempDir() - err := m.Install(&client, repo, io, config.NewBlankConfig()) + m := newTestManager(tempDir, &client, io) + + err := m.Install(repo) assert.Error(t, err) errText := "gh-bin-ext unsupported for windows-amd64. Open an issue: `gh issue create -R owner/gh-bin-ext -t'Support windows-amd64'`" @@ -310,12 +314,12 @@ func TestManager_Install_binary(t *testing.T) { httpmock.REST("GET", "release/cool"), httpmock.StringResponse("FAKE BINARY")) - tempDir := t.TempDir() - m := newTestManager(tempDir) - io, _, _, _ := iostreams.Test() + tempDir := t.TempDir() - err := m.Install(&client, repo, io, config.NewBlankConfig()) + m := newTestManager(tempDir, &client, io) + + err := m.Install(repo) assert.NoError(t, err) manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) @@ -344,7 +348,7 @@ func TestManager_Create(t *testing.T) { oldWd, _ := os.Getwd() assert.NoError(t, os.Chdir(tempDir)) t.Cleanup(func() { _ = os.Chdir(oldWd) }) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) err := m.Create("gh-test") assert.NoError(t, err) files, err := ioutil.ReadDir(filepath.Join(tempDir, "gh-test")) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 961883b12..2ba926033 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -22,7 +22,6 @@ func New(appVersion string) *cmdutil.Factory { Branch: branchFunc(), // No factory dependencies Executable: executable(), // No factory dependencies - ExtensionManager: extension.NewManager(), } f.IOStreams = ioStreams(f) // Depends on Config @@ -30,6 +29,7 @@ func New(appVersion string) *cmdutil.Factory { f.Remotes = remotesFunc(f) // Depends on Config f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes f.Browser = browser(f) // Depends on Config, and IOStreams + f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams return f } @@ -148,6 +148,24 @@ func branchFunc() func() (string, error) { } } +func extensionManager(f *cmdutil.Factory) *extension.Manager { + em := extension.NewManager(f.IOStreams) + + cfg, err := f.Config() + if err != nil { + return em + } + em.SetConfig(cfg) + + client, err := f.HttpClient() + if err != nil { + return em + } + em.SetClient(client) + + return em +} + func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io := iostreams.System() cfg, err := f.Config() diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 16a3c7494..4e9ce89b5 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -2,11 +2,8 @@ package extensions import ( "io" - "net/http" - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/iostreams" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -21,7 +18,7 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error + Install(ghrepo.Interface) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index ce7a71ce8..96d76cdf7 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,11 +4,8 @@ package extensions import ( - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/iostreams" "io" - "net/http" "sync" ) @@ -28,7 +25,7 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +// InstallFunc: func(interfaceMoqParam ghrepo.Interface) error { // panic("mock out the Install method") // }, // InstallLocalFunc: func(dir string) error { @@ -57,7 +54,7 @@ type ExtensionManagerMock struct { DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) // InstallFunc mocks the Install method. - InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error + InstallFunc func(interfaceMoqParam ghrepo.Interface) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -91,14 +88,8 @@ type ExtensionManagerMock struct { } // Install holds details about calls to the Install method. Install []struct { - // Client is the client argument value. - Client *http.Client // InterfaceMoqParam is the interfaceMoqParam argument value. InterfaceMoqParam ghrepo.Interface - // IOStreams is the iOStreams argument value. - IOStreams *iostreams.IOStreams - // ConfigMoqParam is the configMoqParam argument value. - ConfigMoqParam config.Config } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { @@ -211,41 +202,29 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { } // Install calls InstallFunc. -func (mock *ExtensionManagerMock) Install(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +func (mock *ExtensionManagerMock) Install(interfaceMoqParam ghrepo.Interface) error { if mock.InstallFunc == nil { panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") } callInfo := struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config }{ - Client: client, InterfaceMoqParam: interfaceMoqParam, - IOStreams: iOStreams, - ConfigMoqParam: configMoqParam, } mock.lockInstall.Lock() mock.calls.Install = append(mock.calls.Install, callInfo) mock.lockInstall.Unlock() - return mock.InstallFunc(client, interfaceMoqParam, iOStreams, configMoqParam) + return mock.InstallFunc(interfaceMoqParam) } // InstallCalls gets all the calls that were made to Install. // Check the length with: // len(mockedExtensionManager.InstallCalls()) func (mock *ExtensionManagerMock) InstallCalls() []struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config } { var calls []struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config } mock.lockInstall.RLock() calls = mock.calls.Install From 5f02ed2656b5c2846505cb4c6a570f508a9bb7ad Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Sep 2021 15:59:50 -0500 Subject: [PATCH 11/12] linter appeasement --- pkg/cmd/extension/command.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 03466b84d..ad92a8f14 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -5,10 +5,8 @@ import ( "fmt" "os" "strings" - "time" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -103,14 +101,10 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { if err != nil { return err } + if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { return err } - client, err := f.HttpClient() - if err != nil { - return fmt.Errorf("could not make http client: %w", err) - } - client = api.NewCachedClient(client, time.Second*30) return m.Install(repo) }, From 7bf85355a92b80437df7470b53e12a554238d0fc Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Sep 2021 15:59:57 -0500 Subject: [PATCH 12/12] restore cached client --- pkg/cmd/factory/default.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 2ba926033..61bbe2c54 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "os" + "time" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/context" @@ -161,7 +162,8 @@ func extensionManager(f *cmdutil.Factory) *extension.Manager { if err != nil { return em } - em.SetClient(client) + + em.SetClient(api.NewCachedClient(client, time.Second*30)) return em }