CLOUDP-300024: Bug: "plugin warning: could not load plugin"

.golangci.yml

@@ -18,7 +18,6 @@ linters:
     - goconst
     - gocritic
     - gocyclo
-    - godot
     - goprintffuncname
     - gosec
     - govet
@@ -75,6 +74,7 @@ linters:
     # - godox # Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false]
     # - goerr113 # Golang linter to check the errors handling expressions [fast: false, auto-fix: false]
     # - gofumpt # Gofumpt checks whether code was gofumpt-ed. [fast: true, auto-fix: true]
+    # - godot # it doesn't make sense to end every comment with a dot. (Example: lists)
     # - goheader # Checks is file header matches to pattern [fast: true, auto-fix: false]
     # - golint #[deprecated]: Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: false, auto-fix: false]
     # - gomoddirectives # Manage the use of 'replace', 'retract', and 'excludes' directives in go.mod. [fast: true, auto-fix: false]

internal/cli/plugin/first_class.go

@@ -80,8 +80,8 @@ func IsFirstClassPluginCmd(cmd *cobra.Command) bool {
 	return false
 }
 
-func (fcp *FirstClassPlugin) isAlreadyInstalled(plugins []*plugin.Plugin) bool {
-	for _, p := range plugins {
+func (fcp *FirstClassPlugin) isAlreadyInstalled(plugins *plugin.ValidatedPlugins) bool {
+	for _, p := range plugins.GetValidPlugins() {
 		if p.Name == fcp.Name {
 			return true
 		}
@@ -90,8 +90,12 @@ func (fcp *FirstClassPlugin) isAlreadyInstalled(plugins []*plugin.Plugin) bool {
 	return false
 }
 
-func (fcp *FirstClassPlugin) runFirstClassPluginCommand(cmd *cobra.Command, args []string, ghClient *github.Client) error {
-	installOpts := &InstallOpts{}
+func (fcp *FirstClassPlugin) runFirstClassPluginCommand(cmd *cobra.Command, args []string, ghClient *github.Client, plugins *plugin.ValidatedPlugins) error {
+	installOpts := &InstallOpts{
+		Opts: Opts{
+			plugins: plugins,
+		},
+	}
 	installOpts.githubAsset = &GithubAsset{
 		ghClient: ghClient,
 		owner:    fcp.Github.Owner,
@@ -116,7 +120,7 @@ func (fcp *FirstClassPlugin) runFirstClassPluginCommand(cmd *cobra.Command, args
 	return installedPlugin.Run(cmd, args)
 }
 
-func (fcp *FirstClassPlugin) getCommands() []*cobra.Command {
+func (fcp *FirstClassPlugin) getCommands(plugins *plugin.ValidatedPlugins) []*cobra.Command {
 	commands := make([]*cobra.Command, 0, len(fcp.Commands))
 	ghClient := github.NewClient(nil)
 
@@ -129,7 +133,7 @@ func (fcp *FirstClassPlugin) getCommands() []*cobra.Command {
 				sourceType: FirstClassSourceType,
 			},
 			RunE: func(cmd *cobra.Command, args []string) error {
-				return fcp.runFirstClassPluginCommand(cmd, args, ghClient)
+				return fcp.runFirstClassPluginCommand(cmd, args, ghClient, plugins)
 			},
 			DisableFlagParsing: true,
 		}
@@ -140,15 +144,15 @@ func (fcp *FirstClassPlugin) getCommands() []*cobra.Command {
 	return commands
 }
 
-func getFirstClassPluginCommands(plugins []*plugin.Plugin) []*cobra.Command {
+func getFirstClassPluginCommands(plugins *plugin.ValidatedPlugins) []*cobra.Command {
 	var commands []*cobra.Command
 	// create cobra commands to install first class plugins when their commands are run
 	for _, firstClassPlugin := range FirstClassPlugins {
 		if firstClassPlugin.isAlreadyInstalled(plugins) {
 			continue
 		}
 
-		commands = append(commands, firstClassPlugin.getCommands()...)
+		commands = append(commands, firstClassPlugin.getCommands(plugins)...)
 	}
 
 	return commands

internal/cli/plugin/install.go

@@ -58,7 +58,7 @@ func (opts *InstallOpts) validatePlugin(pluginDirectoryPath string) error {
 	}
 
 	// check for duplicate plugin names
-	for _, p := range opts.plugins {
+	for _, p := range opts.getValidPlugins() {
 		if manifest.Name == p.Name {
 			return fmt.Errorf("a plugin with the name %s already exists", manifest.Name)
 		}

internal/cli/plugin/install_test.go

@@ -59,7 +59,9 @@ func Test_checkForDuplicatePlugins(t *testing.T) {
 
 	opts := InstallOpts{
 		Opts: Opts{
-			plugins: plugins,
+			plugins: &plugin.ValidatedPlugins{
+				ValidPlugins: plugins,
+			},
 		},
 	}
 

internal/cli/plugin/list.go

@@ -25,7 +25,7 @@ type ListOps struct {
 }
 
 func (opts *ListOps) Run() error {
-	return opts.Print(opts.plugins)
+	return opts.Print(opts.plugins.GetValidAndInvalidPlugins())
 }
 
 const listTemplate = `NAME	DESCRIPTION	VERSION COMMANDS {{range valueOrEmptySlice .}}

internal/cli/plugin/list_test.go

@@ -43,5 +43,5 @@ func TestList_Run(t *testing.T) {
 }
 
 func TestList_Template(t *testing.T) {
-	test.VerifyOutputTemplate(t, listTemplate, getTestPlugins(t))
+	test.VerifyOutputTemplate(t, listTemplate, getTestPlugins(t).GetValidAndInvalidPlugins())
 }

internal/cli/plugin/plugin.go

@@ -30,10 +30,18 @@ const (
 )
 
 type Opts struct {
-	plugins          []*plugin.Plugin
+	plugins          *plugin.ValidatedPlugins
 	existingCommands []*cobra.Command
 }
 
+func (opts *Opts) getValidPlugins() []*plugin.Plugin {
+	return opts.plugins.GetValidPlugins()
+}
+
+func (opts *Opts) getValidAndInvalidPlugins() []*plugin.Plugin {
+	return opts.plugins.GetValidAndInvalidPlugins()
+}
+
 // find a plugin given the input argument of a plugin command
 // the input arg can be the plugin name, the github values (<repo-owner>/<repo-name>) or the entire github URL of the plugin.
 func (opts *Opts) findPluginWithArg(arg string) (*plugin.Plugin, error) {
@@ -67,28 +75,62 @@ func createExistingCommandsSet(existingCommands []*cobra.Command) set.Set[string
 	return existingCommandsSet
 }
 
+// find a plugin given a github owner and repository name
+//
+// also searches through invalid plugins, because this function is also used for uninstall command
+// throws an error when:
+// - multiple plugins are found with the same github values
+// - no plugin is found with the given github values
 func (opts *Opts) findPluginWithGithubValues(owner string, name string) (*plugin.Plugin, error) {
-	for _, p := range opts.plugins {
+	var foundPlugin *plugin.Plugin
+
+	for _, p := range opts.getValidAndInvalidPlugins() {
 		if p.Github != nil && p.Github.Equals(owner, name) {
-			return p, nil
+			if foundPlugin != nil {
+				return nil, fmt.Errorf(`found multiple plugins with github values %s/%s`, owner, name)
+			}
+
+			foundPlugin = p
 		}
 	}
-	return nil, fmt.Errorf(`could not find plugin with github values %s/%s`, owner, name)
+
+	if foundPlugin == nil {
+		return nil, fmt.Errorf(`could not find plugin with github values %s/%s`, owner, name)
+	}
+
+	return foundPlugin, nil
 }
 
+// find a plugin given a plugin name
+//
+// also searches through invalid plugins, because this function is also used for uninstall command
+// throws an error when:
+// - multiple plugins are found with the same name
+// - no plugin is found with the given name
 func (opts *Opts) findPluginWithName(name string) (*plugin.Plugin, error) {
-	for _, p := range opts.plugins {
+	var foundPlugin *plugin.Plugin
+
+	for _, p := range opts.getValidAndInvalidPlugins() {
 		if p.Name == name {
-			return p, nil
+			if foundPlugin != nil {
+				return nil, fmt.Errorf(`found multiple plugins with name %s`, name)
+			}
+
+			foundPlugin = p
 		}
 	}
-	return nil, fmt.Errorf(`could not find plugin with name %s`, name)
+
+	if foundPlugin == nil {
+		return nil, fmt.Errorf(`could not find plugin with name %s`, name)
+	}
+
+	return foundPlugin, nil
 }
 
 func RegisterCommands(rootCmd *cobra.Command) {
-	plugins := plugin.GetAllValidPlugins(createExistingCommandsSet(rootCmd.Commands()))
+	plugins := plugin.GetAllPluginsValidated(createExistingCommandsSet(rootCmd.Commands()))
 
-	for _, p := range plugins {
+	for _, p := range plugins.GetValidPlugins() {
 		rootCmd.AddCommand(p.GetCobraCommands()...)
 	}
 
@@ -110,7 +152,7 @@ func validateManifest(manifest *plugin.Manifest) error {
 	return nil
 }
 
-func Builder(plugins []*plugin.Plugin, existingCommands []*cobra.Command) *cobra.Command {
+func Builder(plugins *plugin.ValidatedPlugins, existingCommands []*cobra.Command) *cobra.Command {
 	const use = "plugin"
 	cmd := &cobra.Command{
 		Use:     use,

internal/cli/plugin/plugin_test.go

@@ -25,51 +25,94 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func getTestPlugins(t *testing.T) []*plugin.Plugin {
+func getTestPlugins(t *testing.T) *plugin.ValidatedPlugins {
 	t.Helper()
 	version1, err := semver.NewVersion("1.4.5")
 	require.NoError(t, err)
 	version2, err := semver.NewVersion("1.2.3")
 	require.NoError(t, err)
 
-	return []*plugin.Plugin{
-		{
-			Name:        "plugin1",
-			Description: "plugin1 description",
-			Version:     version1,
-			Commands: []*plugin.Command{
-				{Name: "command1"},
-				{Name: "command 2"},
+	return &plugin.ValidatedPlugins{
+		ValidPlugins: []*plugin.Plugin{
+			{
+				Name:        "plugin1",
+				Description: "plugin1 description",
+				Version:     version1,
+				Commands: []*plugin.Command{
+					{Name: "command1"},
+					{Name: "command 2"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner1",
+					Name:  "repo1",
+				},
 			},
-			Github: &plugin.Github{
-				Owner: "owner1",
-				Name:  "repo1",
+			{
+				Name:        "plugin2",
+				Description: "plugin2 description",
+				Version:     version2,
+				Commands: []*plugin.Command{
+					{Name: "command3"},
+					{Name: "command4"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner2",
+					Name:  "repo2",
+				},
 			},
-		},
-		{
-			Name:        "plugin2",
-			Description: "plugin2 description",
-			Version:     version2,
-			Commands: []*plugin.Command{
-				{Name: "command3"},
-				{Name: "command4"},
+			{
+				Name:        "plugin3",
+				Description: "plugin3 description",
+				Version:     version2,
+				Commands: []*plugin.Command{
+					{Name: "command5"},
+					{Name: "command6"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner3",
+					Name:  "repo3",
+				},
 			},
-			Github: &plugin.Github{
-				Owner: "owner2",
-				Name:  "repo2",
+			{
+				Name:        "plugin5",
+				Description: "plugin5 description",
+				Version:     version2,
+				Commands: []*plugin.Command{
+					{Name: "command7"},
+					{Name: "command8"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner5",
+					Name:  "repo5",
+				},
 			},
 		},
-		{
-			Name:        "plugin3",
-			Description: "plugin3 description",
-			Version:     version2,
-			Commands: []*plugin.Command{
-				{Name: "command5"},
-				{Name: "command6"},
+		PluginsWithDuplicateManifestName: []*plugin.Plugin{
+			{
+				Name:        "plugin5",
+				Description: "plugin5 duplicate",
+				Version:     version2,
+				Commands: []*plugin.Command{
+					{Name: "command9"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner5-duplicate",
+					Name:  "repo5-duplicate",
+				},
 			},
-			Github: &plugin.Github{
-				Owner: "owner3",
-				Name:  "repo3",
+		},
+		PluginsWithDuplicateCommands: []*plugin.Plugin{
+			{
+				Name:        "plugin6",
+				Description: "plugin6 description",
+				Version:     version2,
+				Commands: []*plugin.Command{
+					{Name: "command7"},
+				},
+				Github: &plugin.Github{
+					Owner: "owner6",
+					Name:  "repo6",
+				},
 			},
 		},
 	}
@@ -189,6 +232,20 @@ func Test_findPluginWithGithubValues(t *testing.T) {
 			want:      "",
 			wantErr:   true,
 		},
+		{
+			name:      "Plugin found with duplicate name",
+			repoOwner: "owner5-duplicate",
+			repoName:  "repo5-duplicate",
+			want:      "plugin5",
+			wantErr:   false,
+		},
+		{
+			name:      "Plugin found with duplicate command",
+			repoOwner: "owner6",
+			repoName:  "repo6",
+			want:      "plugin6",
+			wantErr:   false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -230,6 +287,18 @@ func Test_findPluginWithName(t *testing.T) {
 			want:     "",
 			wantErr:  true,
 		},
+		{
+			testName: "Plugin found with duplicate name",
+			name:     "plugin5",
+			want:     "",
+			wantErr:  true,
+		},
+		{
+			testName: "Plugin found with duplicate command",
+			name:     "plugin6",
+			want:     "plugin6",
+			wantErr:  false,
+		},
 	}
 
 	for _, tt := range tests {

internal/cli/plugin/update.go

@@ -106,7 +106,7 @@ func (opts *UpdateOpts) validatePlugin(pluginDirectoryPath string) error {
 
 	// make sure that there is exactly one plugin with the same name
 	pluginCount := 0
-	for _, p := range opts.plugins {
+	for _, p := range opts.getValidPlugins() {
 		if manifest.Name == p.Name {
 			pluginCount++
 		}
@@ -203,7 +203,7 @@ func (opts *UpdateOpts) Run(ctx context.Context) error {
 	// if update flag is set, update all plugin, if not update only specified plugin
 	if opts.UpdateAll {
 		// try to create GithubAssetRelease from each plugin -  when create use it to update the plugin
-		for _, p := range opts.plugins {
+		for _, p := range opts.getValidPlugins() {
 			if !p.HasGithub() {
 				continue
 			}