X Tutup
Skip to content

Commit defbf0f

Browse files
samcoesamcoe
committed
Make extension upgrade output more friendly
1 parent 968b093 commit defbf0f

File tree

4 files changed

+191
-119
lines changed

4 files changed

+191
-119
lines changed

‎pkg/cmd/extension/command.go‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,24 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command {
132132
if len(args) > 0 {
133133
name = normalizeExtensionSelector(args[0])
134134
}
135-
return m.Upgrade(name, flagForce)
135+
cs := io.ColorScheme()
136+
err := m.Upgrade(name, flagForce)
137+
if err != nil {
138+
if name != "" {
139+
fmt.Fprintf(io.ErrOut, "%s Failed upgrading extension %s: %s", cs.FailureIcon(), name, err)
140+
} else {
141+
fmt.Fprintf(io.ErrOut, "%s Failed upgrading extensions", cs.FailureIcon())
142+
}
143+
return cmdutil.SilentError
144+
}
145+
if io.IsStdoutTTY() {
146+
if name != "" {
147+
fmt.Fprintf(io.Out, "%s Successfully upgraded extension %s\n", cs.SuccessIcon(), name)
148+
} else {
149+
fmt.Fprintf(io.Out, "%s Successfully upgraded extensions\n", cs.SuccessIcon())
150+
}
151+
}
152+
return nil
136153
},
137154
}
138155
cmd.Flags().BoolVar(&flagAll, "all", false, "Upgrade all extensions")

‎pkg/cmd/extension/command_test.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,23 @@ func TestNewCmdExtension(t *testing.T) {
102102
assert.Equal(t, "hello", calls[0].Name)
103103
}
104104
},
105+
isTTY: true,
106+
wantStdout: "✓ Successfully upgraded extension hello\n",
107+
},
108+
{
109+
name: "upgrade an extension notty",
110+
args: []string{"upgrade", "hello"},
111+
managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
112+
em.UpgradeFunc = func(name string, force bool) error {
113+
return nil
114+
}
115+
return func(t *testing.T) {
116+
calls := em.UpgradeCalls()
117+
assert.Equal(t, 1, len(calls))
118+
assert.Equal(t, "hello", calls[0].Name)
119+
}
120+
},
121+
isTTY: false,
105122
},
106123
{
107124
name: "upgrade an extension gh-prefix",
@@ -116,6 +133,8 @@ func TestNewCmdExtension(t *testing.T) {
116133
assert.Equal(t, "hello", calls[0].Name)
117134
}
118135
},
136+
isTTY: true,
137+
wantStdout: "✓ Successfully upgraded extension hello\n",
119138
},
120139
{
121140
name: "upgrade an extension full name",
@@ -130,6 +149,8 @@ func TestNewCmdExtension(t *testing.T) {
130149
assert.Equal(t, "hello", calls[0].Name)
131150
}
132151
},
152+
isTTY: true,
153+
wantStdout: "✓ Successfully upgraded extension hello\n",
133154
},
134155
{
135156
name: "upgrade all",
@@ -144,6 +165,23 @@ func TestNewCmdExtension(t *testing.T) {
144165
assert.Equal(t, "", calls[0].Name)
145166
}
146167
},
168+
isTTY: true,
169+
wantStdout: "✓ Successfully upgraded extensions\n",
170+
},
171+
{
172+
name: "upgrade all notty",
173+
args: []string{"upgrade", "--all"},
174+
managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
175+
em.UpgradeFunc = func(name string, force bool) error {
176+
return nil
177+
}
178+
return func(t *testing.T) {
179+
calls := em.UpgradeCalls()
180+
assert.Equal(t, 1, len(calls))
181+
assert.Equal(t, "", calls[0].Name)
182+
}
183+
},
184+
isTTY: false,
147185
},
148186
{
149187
name: "remove extension tty",

‎pkg/cmd/extension/manager.go‎

Lines changed: 62 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -417,58 +417,81 @@ func (m *Manager) installGit(cloneURL string, stdout, stderr io.Writer) error {
417417
}
418418

419419
var localExtensionUpgradeError = errors.New("local extensions can not be upgraded")
420+
var upToDateError = errors.New("already up to date")
421+
var noExtensionsInstalledError = errors.New("no extensions installed")
420422

421423
func (m *Manager) Upgrade(name string, force bool) error {
422-
exe, err := m.lookPath("git")
423-
if err != nil {
424-
return err
425-
}
426-
427-
exts := m.List(false)
424+
// Fetch metadata during list only when upgrading all extensions.
425+
// This is a performance improvement so that we don't make a
426+
// bunch of unecessary network requests when trying to upgrade a single extension.
427+
fetchMetadata := name == ""
428+
exts, _ := m.list(fetchMetadata)
428429
if len(exts) == 0 {
429-
return errors.New("no extensions installed")
430+
return noExtensionsInstalledError
431+
}
432+
if name == "" {
433+
return m.upgradeExtensions(exts, force)
430434
}
431-
432-
someUpgraded := false
433435
for _, f := range exts {
434-
if name == "" {
435-
fmt.Fprintf(m.io.Out, "[%s]: ", f.Name())
436-
} else if f.Name() != name {
436+
if f.Name() != name {
437437
continue
438438
}
439-
440-
if f.IsLocal() {
441-
if name == "" {
442-
fmt.Fprintf(m.io.Out, "%s\n", localExtensionUpgradeError)
443-
} else {
444-
err = localExtensionUpgradeError
445-
}
446-
continue
439+
var err error
440+
// For single extensions manually retrieve latest version since we forgo
441+
// doing it during list.
442+
f.latestVersion, err = m.getLatestVersion(f)
443+
if err != nil {
444+
return err
447445
}
446+
return m.upgradeExtension(f, force)
447+
}
448+
return fmt.Errorf("no extension matched %q", name)
449+
}
448450

449-
binManifestPath := filepath.Join(filepath.Dir(f.Path()), manifestName)
450-
if _, e := os.Stat(binManifestPath); e == nil {
451-
err = m.upgradeBin(f)
452-
someUpgraded = true
451+
func (m *Manager) upgradeExtensions(exts []Extension, force bool) error {
452+
var failed bool
453+
for _, f := range exts {
454+
fmt.Fprintf(m.io.Out, "[%s]: ", f.Name())
455+
err := m.upgradeExtension(f, force)
456+
if err != nil {
457+
if !errors.Is(err, localExtensionUpgradeError) &&
458+
!errors.Is(err, upToDateError) {
459+
failed = true
460+
}
461+
fmt.Fprintf(m.io.Out, "%s\n", err)
453462
continue
454463
}
455-
456-
if e := m.upgradeGit(f, exe, force); e != nil {
457-
err = e
458-
}
459-
someUpgraded = true
464+
fmt.Fprintf(m.io.Out, "upgrade complete\n")
460465
}
461-
462-
if err == nil && !someUpgraded {
463-
err = fmt.Errorf("no extension matched %q", name)
466+
if failed {
467+
return errors.New("some extensions failed to upgrade")
464468
}
469+
return nil
470+
}
465471

472+
func (m *Manager) upgradeExtension(ext Extension, force bool) error {
473+
if ext.isLocal {
474+
return localExtensionUpgradeError
475+
}
476+
if !ext.UpdateAvailable() {
477+
return upToDateError
478+
}
479+
var err error
480+
if ext.kind == BinaryKind {
481+
err = m.upgradeBinExtension(ext)
482+
} else {
483+
err = m.upgradeGitExtension(ext, force)
484+
}
466485
return err
467486
}
468487

469-
func (m *Manager) upgradeGit(ext extensions.Extension, exe string, force bool) error {
488+
func (m *Manager) upgradeGitExtension(ext Extension, force bool) error {
489+
exe, err := m.lookPath("git")
490+
if err != nil {
491+
return err
492+
}
470493
var cmds []*exec.Cmd
471-
dir := filepath.Dir(ext.Path())
494+
dir := filepath.Dir(ext.path)
472495
if force {
473496
fetchCmd := m.newCommand(exe, "-C", dir, "--git-dir="+filepath.Join(dir, ".git"), "fetch", "origin", "HEAD")
474497
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
477500
pullCmd := m.newCommand(exe, "-C", dir, "--git-dir="+filepath.Join(dir, ".git"), "pull", "--ff-only")
478501
cmds = []*exec.Cmd{pullCmd}
479502
}
480-
481-
return runCmds(cmds, m.io.Out, m.io.ErrOut)
503+
return runCmds(cmds)
482504
}
483505

484-
func (m *Manager) upgradeBin(ext extensions.Extension) error {
485-
manifestPath := filepath.Join(filepath.Dir(ext.Path()), manifestName)
486-
manifest, err := os.ReadFile(manifestPath)
506+
func (m *Manager) upgradeBinExtension(ext Extension) error {
507+
repo, err := ghrepo.FromFullName(ext.url)
487508
if err != nil {
488-
return fmt.Errorf("could not open %s for reading: %w", manifestPath, err)
509+
return fmt.Errorf("failed to parse URL %s: %w", ext.url, err)
489510
}
490-
491-
var bm binManifest
492-
err = yaml.Unmarshal(manifest, &bm)
493-
if err != nil {
494-
return fmt.Errorf("could not parse %s: %w", manifestPath, err)
495-
}
496-
repo := ghrepo.NewWithHost(bm.Owner, bm.Name, bm.Host)
497-
var r *release
498-
499-
r, err = fetchLatestRelease(m.client, repo)
500-
if err != nil {
501-
return fmt.Errorf("failed to get release info for %s: %w", ghrepo.FullName(repo), err)
502-
}
503-
504-
if bm.Tag == r.Tag {
505-
return nil
506-
}
507-
508511
return m.installBin(repo)
509512
}
510513

@@ -590,10 +593,8 @@ func (m *Manager) Create(name string) error {
590593
return err
591594
}
592595

593-
func runCmds(cmds []*exec.Cmd, stdout, stderr io.Writer) error {
596+
func runCmds(cmds []*exec.Cmd) error {
594597
for _, cmd := range cmds {
595-
cmd.Stdout = stdout
596-
cmd.Stderr = stderr
597598
if err := cmd.Run(); err != nil {
598599
return err
599600
}

0 commit comments

Comments
 (0)
X Tutup