Merge pull request #3941 from cli/extension-install-check

Extension install check

pkg/cmd/extensions/command.go

@@ -10,6 +10,7 @@ import (
 	"github.com/cli/cli/git"
 	"github.com/cli/cli/internal/ghrepo"
 	"github.com/cli/cli/pkg/cmdutil"
+	"github.com/cli/cli/pkg/extensions"
 	"github.com/cli/cli/utils"
 	"github.com/spf13/cobra"
 )
@@ -73,13 +74,15 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command {
 					}
 					return m.InstallLocal(wd)
 				}
+
 				repo, err := ghrepo.FromFullName(args[0])
 				if err != nil {
 					return err
 				}
-				if !strings.HasPrefix(repo.RepoName(), "gh-") {
-					return errors.New("the repository name must start with `gh-`")
+				if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil {
+					return err
 				}
+
 				cfg, err := f.Config()
 				if err != nil {
 					return err
@@ -129,3 +132,24 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command {
 	extCmd.Hidden = true
 	return &extCmd
 }
+
+func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, extName string) error {
+	if !strings.HasPrefix(extName, "gh-") {
+		return errors.New("extension repository name must start with `gh-`")
+	}
+
+	commandName := strings.TrimPrefix(extName, "gh-")
+	if c, _, err := rootCmd.Traverse([]string{commandName}); err != nil {
+		return err
+	} else if c != rootCmd {
+		return fmt.Errorf("%q matches the name of a built-in command", commandName)
+	}
+
+	for _, ext := range m.List() {
+		if ext.Name() == commandName {
+			return fmt.Errorf("there is already an installed extension that provides the %q command", commandName)
+		}
+	}
+
+	return nil
+}

pkg/cmd/extensions/command_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/cli/cli/pkg/cmdutil"
 	"github.com/cli/cli/pkg/extensions"
 	"github.com/cli/cli/pkg/iostreams"
+	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -25,23 +26,43 @@ func TestNewCmdExtensions(t *testing.T) {
 		args         []string
 		managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T)
 		wantErr      bool
-		wantStdout   string
-		wantStderr   string
+		errMsg       string
 	}{
 		{
 			name: "install an extension",
 			args: []string{"install", "owner/gh-some-ext"},
 			managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
+				em.ListFunc = func() []extensions.Extension {
+					return []extensions.Extension{}
+				}
 				em.InstallFunc = func(s string, out, errOut io.Writer) error {
 					return nil
 				}
 				return func(t *testing.T) {
-					calls := em.InstallCalls()
-					assert.Equal(t, 1, len(calls))
-					assert.Equal(t, "https://github.com/owner/gh-some-ext.git", calls[0].URL)
+					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 an extension with same name as existing extension",
+			args: []string{"install", "owner/gh-existing-ext"},
+			managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
+				em.ListFunc = func() []extensions.Extension {
+					e := &Extension{path: "owner2/gh-existing-ext"}
+					return []extensions.Extension{e}
+				}
+				return func(t *testing.T) {
+					calls := em.ListCalls()
+					assert.Equal(t, 1, len(calls))
+				}
+			},
+			wantErr: true,
+			errMsg:  "there is already an installed extension that provides the \"existing-ext\" command",
+		},
 		{
 			name: "install local extension",
 			args: []string{"install", "."},
@@ -60,6 +81,7 @@ func TestNewCmdExtensions(t *testing.T) {
 			name:    "upgrade error",
 			args:    []string{"upgrade"},
 			wantErr: true,
+			errMsg:  "must specify an extension to upgrade",
 		},
 		{
 			name: "upgrade an extension",
@@ -107,7 +129,7 @@ func TestNewCmdExtensions(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			ios, _, stdout, stderr := iostreams.Test()
+			ios, _, _, _ := iostreams.Test()
 
 			var assertFunc func(*testing.T)
 			em := &extensions.ExtensionManagerMock{}
@@ -130,7 +152,7 @@ func TestNewCmdExtensions(t *testing.T) {
 
 			_, err := cmd.ExecuteC()
 			if tt.wantErr {
-				assert.Error(t, err)
+				assert.EqualError(t, err, tt.errMsg)
 			} else {
 				assert.NoError(t, err)
 			}
@@ -138,9 +160,6 @@ func TestNewCmdExtensions(t *testing.T) {
 			if assertFunc != nil {
 				assertFunc(t)
 			}
-
-			assert.Equal(t, tt.wantStdout, stdout.String())
-			assert.Equal(t, tt.wantStderr, stderr.String())
 		})
 	}
 }
@@ -148,3 +167,79 @@ func TestNewCmdExtensions(t *testing.T) {
 func normalizeDir(d string) string {
 	return strings.TrimPrefix(d, "/private")
 }
+
+func Test_checkValidExtension(t *testing.T) {
+	rootCmd := &cobra.Command{}
+	rootCmd.AddCommand(&cobra.Command{Use: "help"})
+	rootCmd.AddCommand(&cobra.Command{Use: "auth"})
+
+	m := &extensions.ExtensionManagerMock{
+		ListFunc: func() []extensions.Extension {
+			return []extensions.Extension{
+				&extensions.ExtensionMock{
+					NameFunc: func() string { return "screensaver" },
+				},
+				&extensions.ExtensionMock{
+					NameFunc: func() string { return "triage" },
+				},
+			}
+		},
+	}
+
+	type args struct {
+		rootCmd *cobra.Command
+		manager extensions.ExtensionManager
+		extName string
+	}
+	tests := []struct {
+		name      string
+		args      args
+		wantError string
+	}{
+		{
+			name: "valid extension",
+			args: args{
+				rootCmd: rootCmd,
+				manager: m,
+				extName: "gh-hello",
+			},
+		},
+		{
+			name: "invalid extension name",
+			args: args{
+				rootCmd: rootCmd,
+				manager: m,
+				extName: "gherkins",
+			},
+			wantError: "extension repository name must start with `gh-`",
+		},
+		{
+			name: "clashes with built-in command",
+			args: args{
+				rootCmd: rootCmd,
+				manager: m,
+				extName: "gh-auth",
+			},
+			wantError: "\"auth\" matches the name of a built-in command",
+		},
+		{
+			name: "clashes with an installed extension",
+			args: args{
+				rootCmd: rootCmd,
+				manager: m,
+				extName: "gh-triage",
+			},
+			wantError: "there is already an installed extension that provides the \"triage\" command",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := checkValidExtension(tt.args.rootCmd, tt.args.manager, tt.args.extName)
+			if tt.wantError == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.EqualError(t, err, tt.wantError)
+			}
+		})
+	}
+}