From 42ce8faafa85b9903db88e2d4236c53f1ccc5114 Mon Sep 17 00:00:00 2001 From: nate smith Date: Tue, 19 Oct 2021 14:15:48 -0500 Subject: [PATCH 1/3] dispatch binary extensions directly --- pkg/cmd/extension/extension.go | 4 ++++ pkg/cmd/extension/manager.go | 8 +++++-- pkg/extensions/extension.go | 1 + pkg/extensions/extension_mock.go | 36 ++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/extension/extension.go b/pkg/cmd/extension/extension.go index ead9204cd..b4106228f 100644 --- a/pkg/cmd/extension/extension.go +++ b/pkg/cmd/extension/extension.go @@ -48,3 +48,7 @@ func (e *Extension) UpdateAvailable() bool { } return true } + +func (e *Extension) IsBinary() bool { + return e.kind == BinaryKind +} diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index aec54f5b2..7110254a4 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -69,9 +69,11 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri forwardArgs := args[1:] exts, _ := m.list(false) + var ext Extension for _, e := range exts { if e.Name() == extName { - exe = e.Path() + ext = e + exe = ext.Path() break } } @@ -81,7 +83,9 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri var externalCmd *exec.Cmd - if runtime.GOOS == "windows" { + if ext.IsBinary() { + externalCmd = m.newCommand(exe, forwardArgs...) + } else if runtime.GOOS == "windows" { // Dispatch all extension calls through the `sh` interpreter to support executable files with a // shebang line on Windows. shExe, err := m.findSh() diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index f0962a87a..ee55bfabc 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -13,6 +13,7 @@ type Extension interface { URL() string IsLocal() bool UpdateAvailable() bool + IsBinary() bool } //go:generate moq -rm -out manager_mock.go . ExtensionManager diff --git a/pkg/extensions/extension_mock.go b/pkg/extensions/extension_mock.go index 17e2a3f6b..b999ab955 100644 --- a/pkg/extensions/extension_mock.go +++ b/pkg/extensions/extension_mock.go @@ -17,6 +17,9 @@ var _ Extension = &ExtensionMock{} // // // make and configure a mocked Extension // mockedExtension := &ExtensionMock{ +// IsBinaryFunc: func() bool { +// panic("mock out the IsBinary method") +// }, // IsLocalFunc: func() bool { // panic("mock out the IsLocal method") // }, @@ -39,6 +42,9 @@ var _ Extension = &ExtensionMock{} // // } type ExtensionMock struct { + // IsBinaryFunc mocks the IsBinary method. + IsBinaryFunc func() bool + // IsLocalFunc mocks the IsLocal method. IsLocalFunc func() bool @@ -56,6 +62,9 @@ type ExtensionMock struct { // calls tracks calls to the methods. calls struct { + // IsBinary holds details about calls to the IsBinary method. + IsBinary []struct { + } // IsLocal holds details about calls to the IsLocal method. IsLocal []struct { } @@ -72,6 +81,7 @@ type ExtensionMock struct { UpdateAvailable []struct { } } + lockIsBinary sync.RWMutex lockIsLocal sync.RWMutex lockName sync.RWMutex lockPath sync.RWMutex @@ -79,6 +89,32 @@ type ExtensionMock struct { lockUpdateAvailable sync.RWMutex } +// IsBinary calls IsBinaryFunc. +func (mock *ExtensionMock) IsBinary() bool { + if mock.IsBinaryFunc == nil { + panic("ExtensionMock.IsBinaryFunc: method is nil but Extension.IsBinary was just called") + } + callInfo := struct { + }{} + mock.lockIsBinary.Lock() + mock.calls.IsBinary = append(mock.calls.IsBinary, callInfo) + mock.lockIsBinary.Unlock() + return mock.IsBinaryFunc() +} + +// IsBinaryCalls gets all the calls that were made to IsBinary. +// Check the length with: +// len(mockedExtension.IsBinaryCalls()) +func (mock *ExtensionMock) IsBinaryCalls() []struct { +} { + var calls []struct { + } + mock.lockIsBinary.RLock() + calls = mock.calls.IsBinary + mock.lockIsBinary.RUnlock() + return calls +} + // IsLocal calls IsLocalFunc. func (mock *ExtensionMock) IsLocal() bool { if mock.IsLocalFunc == nil { From c696416a118b3adec106fa49a95841b80b310826 Mon Sep 17 00:00:00 2001 From: nate smith Date: Tue, 19 Oct 2021 14:52:14 -0500 Subject: [PATCH 2/3] add test --- pkg/cmd/extension/manager_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 121f47765..e46a30ef5 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -139,6 +139,30 @@ func TestManager_Dispatch(t *testing.T) { assert.Equal(t, "", stderr.String()) } +func TestManager_Dispatch_binary(t *testing.T) { + tempDir := t.TempDir() + extPath := filepath.Join(tempDir, "extensions", "gh-hello") + exePath := filepath.Join(extPath, "gh-hello") + bm := binManifest{ + Owner: "owner", + Name: "gh-hello", + Host: "github.com", + Tag: "v1.0.0", + } + assert.NoError(t, stubBinaryExtension(extPath, bm)) + + m := newTestManager(tempDir, nil, nil) + + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + found, err := m.Dispatch([]string{"hello", "one", "two"}, nil, stdout, stderr) + assert.NoError(t, err) + assert.True(t, found) + + assert.Equal(t, fmt.Sprintf("[%s one two]\n", exePath), stdout.String()) + assert.Equal(t, "", stderr.String()) +} + func TestManager_Remove(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) From f65c1537dc37e742e378a9d7155bd316315f8794 Mon Sep 17 00:00:00 2001 From: nate smith Date: Thu, 21 Oct 2021 11:58:25 -0500 Subject: [PATCH 3/3] review feedback --- pkg/cmd/extension/manager.go | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 7110254a4..f06594d05 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -83,7 +83,7 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri var externalCmd *exec.Cmd - if ext.IsBinary() { + if ext.IsBinary() || runtime.GOOS != "windows" { externalCmd = m.newCommand(exe, forwardArgs...) } else if runtime.GOOS == "windows" { // Dispatch all extension calls through the `sh` interpreter to support executable files with a @@ -97,8 +97,6 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri } forwardArgs = append([]string{"-c", `command "$@"`, "--", exe}, forwardArgs...) externalCmd = m.newCommand(shExe, forwardArgs...) - } else { - externalCmd = m.newCommand(exe, forwardArgs...) } externalCmd.Stdin = stdin externalCmd.Stdout = stdout @@ -272,7 +270,17 @@ func (m *Manager) getLatestVersion(ext Extension) (string, error) { if ext.isLocal { return "", fmt.Errorf("unable to get latest version for local extensions") } - if ext.kind == GitKind { + if ext.IsBinary() { + repo, err := ghrepo.FromFullName(ext.url) + if err != nil { + return "", err + } + r, err := fetchLatestRelease(m.client, repo) + if err != nil { + return "", err + } + return r.Tag, nil + } else { gitExe, err := m.lookPath("git") if err != nil { return "", err @@ -286,16 +294,6 @@ func (m *Manager) getLatestVersion(ext Extension) (string, error) { } remoteSha := bytes.SplitN(lsRemote, []byte("\t"), 2)[0] return string(remoteSha), nil - } else { - repo, err := ghrepo.FromFullName(ext.url) - if err != nil { - return "", err - } - r, err := fetchLatestRelease(m.client, repo) - if err != nil { - return "", err - } - return r.Tag, nil } } @@ -481,7 +479,7 @@ func (m *Manager) upgradeExtension(ext Extension, force bool) error { return upToDateError } var err error - if ext.kind == BinaryKind { + if ext.IsBinary() { err = m.upgradeBinExtension(ext) } else { err = m.upgradeGitExtension(ext, force)