diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 264945026..7b51e1498 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -132,7 +132,24 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { if len(args) > 0 { name = normalizeExtensionSelector(args[0]) } - return m.Upgrade(name, flagForce) + cs := io.ColorScheme() + err := m.Upgrade(name, flagForce) + if err != nil { + if name != "" { + fmt.Fprintf(io.ErrOut, "%s Failed upgrading extension %s: %s", cs.FailureIcon(), name, err) + } else { + fmt.Fprintf(io.ErrOut, "%s Failed upgrading extensions", cs.FailureIcon()) + } + return cmdutil.SilentError + } + if io.IsStdoutTTY() { + if name != "" { + fmt.Fprintf(io.Out, "%s Successfully upgraded extension %s\n", cs.SuccessIcon(), name) + } else { + fmt.Fprintf(io.Out, "%s Successfully upgraded extensions\n", cs.SuccessIcon()) + } + } + return nil }, } cmd.Flags().BoolVar(&flagAll, "all", false, "Upgrade all extensions") diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index f1529a63d..839484c3e 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -102,6 +102,23 @@ func TestNewCmdExtension(t *testing.T) { assert.Equal(t, "hello", calls[0].Name) } }, + isTTY: true, + wantStdout: "✓ Successfully upgraded extension hello\n", + }, + { + name: "upgrade an extension notty", + args: []string{"upgrade", "hello"}, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.UpgradeFunc = func(name string, force bool) error { + return nil + } + return func(t *testing.T) { + calls := em.UpgradeCalls() + assert.Equal(t, 1, len(calls)) + assert.Equal(t, "hello", calls[0].Name) + } + }, + isTTY: false, }, { name: "upgrade an extension gh-prefix", @@ -116,6 +133,8 @@ func TestNewCmdExtension(t *testing.T) { assert.Equal(t, "hello", calls[0].Name) } }, + isTTY: true, + wantStdout: "✓ Successfully upgraded extension hello\n", }, { name: "upgrade an extension full name", @@ -130,6 +149,8 @@ func TestNewCmdExtension(t *testing.T) { assert.Equal(t, "hello", calls[0].Name) } }, + isTTY: true, + wantStdout: "✓ Successfully upgraded extension hello\n", }, { name: "upgrade all", @@ -144,6 +165,23 @@ func TestNewCmdExtension(t *testing.T) { assert.Equal(t, "", calls[0].Name) } }, + isTTY: true, + wantStdout: "✓ Successfully upgraded extensions\n", + }, + { + name: "upgrade all notty", + args: []string{"upgrade", "--all"}, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.UpgradeFunc = func(name string, force bool) error { + return nil + } + return func(t *testing.T) { + calls := em.UpgradeCalls() + assert.Equal(t, 1, len(calls)) + assert.Equal(t, "", calls[0].Name) + } + }, + isTTY: false, }, { name: "remove extension tty", diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 68071ced0..aec54f5b2 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -417,58 +417,81 @@ func (m *Manager) installGit(cloneURL string, stdout, stderr io.Writer) error { } var localExtensionUpgradeError = errors.New("local extensions can not be upgraded") +var upToDateError = errors.New("already up to date") +var noExtensionsInstalledError = errors.New("no extensions installed") func (m *Manager) Upgrade(name string, force bool) error { + // Fetch metadata during list only when upgrading all extensions. + // This is a performance improvement so that we don't make a + // bunch of unecessary network requests when trying to upgrade a single extension. + fetchMetadata := name == "" + exts, _ := m.list(fetchMetadata) + if len(exts) == 0 { + return noExtensionsInstalledError + } + if name == "" { + return m.upgradeExtensions(exts, force) + } + for _, f := range exts { + if f.Name() != name { + continue + } + var err error + // For single extensions manually retrieve latest version since we forgo + // doing it during list. + f.latestVersion, err = m.getLatestVersion(f) + if err != nil { + return err + } + return m.upgradeExtension(f, force) + } + return fmt.Errorf("no extension matched %q", name) +} + +func (m *Manager) upgradeExtensions(exts []Extension, force bool) error { + var failed bool + for _, f := range exts { + fmt.Fprintf(m.io.Out, "[%s]: ", f.Name()) + err := m.upgradeExtension(f, force) + if err != nil { + if !errors.Is(err, localExtensionUpgradeError) && + !errors.Is(err, upToDateError) { + failed = true + } + fmt.Fprintf(m.io.Out, "%s\n", err) + continue + } + fmt.Fprintf(m.io.Out, "upgrade complete\n") + } + if failed { + return errors.New("some extensions failed to upgrade") + } + return nil +} + +func (m *Manager) upgradeExtension(ext Extension, force bool) error { + if ext.isLocal { + return localExtensionUpgradeError + } + if !ext.UpdateAvailable() { + return upToDateError + } + var err error + if ext.kind == BinaryKind { + err = m.upgradeBinExtension(ext) + } else { + err = m.upgradeGitExtension(ext, force) + } + return err +} + +func (m *Manager) upgradeGitExtension(ext Extension, force bool) error { exe, err := m.lookPath("git") if err != nil { return err } - - exts := m.List(false) - if len(exts) == 0 { - return errors.New("no extensions installed") - } - - someUpgraded := false - for _, f := range exts { - if name == "" { - fmt.Fprintf(m.io.Out, "[%s]: ", f.Name()) - } else if f.Name() != name { - continue - } - - if f.IsLocal() { - if name == "" { - fmt.Fprintf(m.io.Out, "%s\n", localExtensionUpgradeError) - } else { - err = localExtensionUpgradeError - } - continue - } - - binManifestPath := filepath.Join(filepath.Dir(f.Path()), manifestName) - if _, e := os.Stat(binManifestPath); e == nil { - err = m.upgradeBin(f) - someUpgraded = true - continue - } - - if e := m.upgradeGit(f, exe, force); e != nil { - err = e - } - someUpgraded = true - } - - if err == nil && !someUpgraded { - err = fmt.Errorf("no extension matched %q", name) - } - - return err -} - -func (m *Manager) upgradeGit(ext extensions.Extension, exe string, force bool) error { var cmds []*exec.Cmd - dir := filepath.Dir(ext.Path()) + dir := filepath.Dir(ext.path) if force { fetchCmd := m.newCommand(exe, "-C", dir, "--git-dir="+filepath.Join(dir, ".git"), "fetch", "origin", "HEAD") resetCmd := m.newCommand(exe, "-C", dir, "--git-dir="+filepath.Join(dir, ".git"), "reset", "--hard", "origin/HEAD") @@ -477,34 +500,14 @@ func (m *Manager) upgradeGit(ext extensions.Extension, exe string, force bool) e pullCmd := m.newCommand(exe, "-C", dir, "--git-dir="+filepath.Join(dir, ".git"), "pull", "--ff-only") cmds = []*exec.Cmd{pullCmd} } - - return runCmds(cmds, m.io.Out, m.io.ErrOut) + return runCmds(cmds) } -func (m *Manager) upgradeBin(ext extensions.Extension) error { - manifestPath := filepath.Join(filepath.Dir(ext.Path()), manifestName) - manifest, err := os.ReadFile(manifestPath) +func (m *Manager) upgradeBinExtension(ext Extension) error { + repo, err := ghrepo.FromFullName(ext.url) if err != nil { - return fmt.Errorf("could not open %s for reading: %w", manifestPath, err) + return fmt.Errorf("failed to parse URL %s: %w", ext.url, err) } - - var bm binManifest - err = yaml.Unmarshal(manifest, &bm) - if err != nil { - return fmt.Errorf("could not parse %s: %w", manifestPath, err) - } - repo := ghrepo.NewWithHost(bm.Owner, bm.Name, bm.Host) - var r *release - - r, err = fetchLatestRelease(m.client, repo) - if err != nil { - return fmt.Errorf("failed to get release info for %s: %w", ghrepo.FullName(repo), err) - } - - if bm.Tag == r.Tag { - return nil - } - return m.installBin(repo) } @@ -590,10 +593,8 @@ func (m *Manager) Create(name string) error { return err } -func runCmds(cmds []*exec.Cmd, stdout, stderr io.Writer) error { +func runCmds(cmds []*exec.Cmd) error { for _, cmd := range cmds { - cmd.Stdout = stdout - cmd.Stderr = stderr if err := cmd.Run(); err != nil { return err } diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 3d613633b..121f47765 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -42,6 +42,10 @@ func newTestManager(dir string, client *http.Client, io *iostreams.IOStreams) *M newCommand: func(exe string, args ...string) *exec.Cmd { args = append([]string{os.Args[0], "-test.run=TestHelperProcess", "--", exe}, args...) cmd := exec.Command(args[0], args[1:]...) + if io != nil { + cmd.Stdout = io.Out + cmd.Stderr = io.ErrOut + } cmd.Env = []string{"GH_WANT_HELPER_PROCESS=1"} return cmd }, @@ -150,24 +154,50 @@ func TestManager_Remove(t *testing.T) { assert.Equal(t, "gh-two", items[0].Name()) } -func TestManager_Upgrade_AllExtensions(t *testing.T) { +func TestManager_Upgrade_NoExtensions(t *testing.T) { + tempDir := t.TempDir() + io, _, stdout, stderr := iostreams.Test() + m := newTestManager(tempDir, nil, io) + err := m.Upgrade("", false) + assert.EqualError(t, err, "no extensions installed") + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "", stderr.String()) +} + +func TestManager_Upgrade_NoMatchingExtension(t *testing.T) { + tempDir := t.TempDir() + assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) + io, _, stdout, stderr := iostreams.Test() + m := newTestManager(tempDir, nil, io) + err := m.Upgrade("invalid", false) + assert.EqualError(t, err, `no extension matched "invalid"`) + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "", stderr.String()) +} + +func TestManager_UpgradeExtensions(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) 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"))) - io, _, stdout, stderr := iostreams.Test() - m := newTestManager(tempDir, nil, io) - - err := m.Upgrade("", false) + exts, err := m.list(false) + assert.NoError(t, err) + assert.Equal(t, 3, len(exts)) + for i := 0; i < 3; i++ { + exts[i].currentVersion = "old version" + exts[i].latestVersion = "new version" + } + err = m.upgradeExtensions(exts, false) assert.NoError(t, err) - assert.Equal(t, heredoc.Docf( ` [hello]: [git -C %s --git-dir=%s pull --ff-only] + upgrade complete [local]: local extensions can not be upgraded [two]: [git -C %s --git-dir=%s pull --ff-only] + upgrade complete `, filepath.Join(tempDir, "extensions", "gh-hello"), filepath.Join(tempDir, "extensions", "gh-hello", ".git"), @@ -177,15 +207,33 @@ func TestManager_Upgrade_AllExtensions(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_Upgrade_RemoteExtension(t *testing.T) { +func TestManager_UpgradeExtension_LocalExtension(t *testing.T) { tempDir := t.TempDir() - assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) + assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) io, _, stdout, stderr := iostreams.Test() - m := newTestManager(tempDir, nil, io) + exts, err := m.list(false) + assert.NoError(t, err) + assert.Equal(t, 1, len(exts)) + err = m.upgradeExtension(exts[0], false) + assert.EqualError(t, err, "local extensions can not be upgraded") + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "", stderr.String()) +} - err := m.Upgrade("remote", false) +func TestManager_UpgradeExtension_GitExtension(t *testing.T) { + tempDir := t.TempDir() + assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) + io, _, stdout, stderr := iostreams.Test() + m := newTestManager(tempDir, nil, io) + exts, err := m.list(false) + assert.NoError(t, err) + assert.Equal(t, 1, len(exts)) + ext := exts[0] + ext.currentVersion = "old version" + ext.latestVersion = "new version" + err = m.upgradeExtension(ext, false) assert.NoError(t, err) assert.Equal(t, heredoc.Docf( ` @@ -197,30 +245,20 @@ func TestManager_Upgrade_RemoteExtension(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_Upgrade_LocalExtension(t *testing.T) { - tempDir := t.TempDir() - assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) - - io, _, stdout, stderr := iostreams.Test() - m := newTestManager(tempDir, nil, io) - - err := m.Upgrade("local", false) - assert.EqualError(t, err, "local extensions can not be upgraded") - assert.Equal(t, "", stdout.String()) - assert.Equal(t, "", stderr.String()) -} - -func TestManager_Upgrade_Force(t *testing.T) { +func TestManager_UpgradeExtension_GitExtension_Force(t *testing.T) { tempDir := t.TempDir() extensionDir := filepath.Join(tempDir, "extensions", "gh-remote") gitDir := filepath.Join(tempDir, "extensions", "gh-remote", ".git") - assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) - io, _, stdout, stderr := iostreams.Test() m := newTestManager(tempDir, nil, io) - - err := m.Upgrade("remote", true) + exts, err := m.list(false) + assert.NoError(t, err) + assert.Equal(t, 1, len(exts)) + ext := exts[0] + ext.currentVersion = "old version" + ext.latestVersion = "new version" + err = m.upgradeExtension(ext, true) assert.NoError(t, err) assert.Equal(t, heredoc.Docf( ` @@ -235,26 +273,12 @@ func TestManager_Upgrade_Force(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_Upgrade_NoExtensions(t *testing.T) { +func TestManager_UpgradeExtension_BinaryExtension(t *testing.T) { tempDir := t.TempDir() - - io, _, stdout, stderr := iostreams.Test() - m := newTestManager(tempDir, nil, io) - - err := m.Upgrade("", false) - assert.EqualError(t, err, "no extensions installed") - assert.Equal(t, "", stdout.String()) - assert.Equal(t, "", stderr.String()) -} - -func TestManager_Upgrade_BinaryExtension(t *testing.T) { - tempDir := t.TempDir() - io, _, _, _ := iostreams.Test() reg := httpmock.Registry{} defer reg.Verify(t) client := http.Client{Transport: ®} - assert.NoError(t, stubBinaryExtension( filepath.Join(tempDir, "extensions", "gh-bin-ext"), binManifest{ @@ -263,20 +287,7 @@ func TestManager_Upgrade_BinaryExtension(t *testing.T) { Host: "example.com", Tag: "v1.0.1", })) - m := newTestManager(tempDir, &client, io) - reg.Register( - httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), - httpmock.JSONResponse( - release{ - Tag: "v1.0.2", - Assets: []releaseAsset{ - { - Name: "gh-bin-ext-windows-amd64", - APIURL: "https://example.com/release/cool2", - }, - }, - })) reg.Register( httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), httpmock.JSONResponse( @@ -293,7 +304,12 @@ func TestManager_Upgrade_BinaryExtension(t *testing.T) { httpmock.REST("GET", "release/cool2"), httpmock.StringResponse("FAKE UPGRADED BINARY")) - err := m.Upgrade("bin-ext", false) + exts, err := m.list(false) + assert.NoError(t, err) + assert.Equal(t, 1, len(exts)) + ext := exts[0] + ext.latestVersion = "v1.0.2" + err = m.upgradeExtension(ext, false) assert.NoError(t, err) manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext", manifestName))