fix : backward compatibility in loading yaml plugins (#567)

* fix : load both yaml and bin plugin if available, revert plugin discovery logic

* add tests

Author: smarchone

cmd/version/version.go

@@ -115,11 +115,7 @@ func (v *versionCommand) printAllPluginInfos() {
 	v.logger.Info(fmt.Sprintf("\nDiscovered plugins: %d", len(plugins)))
 	for taskIdx, tasks := range plugins {
 		schema := tasks.Info()
-		if tasks.YamlMod != nil {
-			v.logger.Info(fmt.Sprintf("\n%d. %s (%s)", taskIdx+1, schema.Name, "yaml"))
-		} else {
-			v.logger.Info(fmt.Sprintf("\n%d. %s", taskIdx+1, schema.Name))
-		}
+		v.logger.Info(fmt.Sprintf("\n%d. %s", taskIdx+1, schema.Name))
 		v.logger.Info(fmt.Sprintf("Description: %s", schema.Description))
 		v.logger.Info(fmt.Sprintf("Image: %s", schema.Image))
 		v.logger.Info(fmt.Sprintf("Type: %s", schema.PluginType))

models/plugin.go

@@ -526,13 +526,13 @@ func (s *registeredPlugins) Add(baseMod BasePlugin, cliMod CommandLineMod, drMod
 func (s *registeredPlugins) add(baseMod BasePlugin, cliMod CommandLineMod, drMod DependencyResolverMod, yamlMod CommandLineMod) error {
 	var info *PluginInfoResponse
 	var err error
-	if baseMod != nil {
-		info, err = baseMod.PluginInfo()
+	if yamlMod != nil {
+		info, err = yamlMod.PluginInfo()
 		if err != nil {
 			return err
 		}
 	} else {
-		info, err = yamlMod.PluginInfo()
+		info, err = baseMod.PluginInfo()
 		if err != nil {
 			return err
 		}
@@ -542,11 +542,6 @@ func (s *registeredPlugins) add(baseMod BasePlugin, cliMod CommandLineMod, drMod
 		return errors.New("plugin name cannot be empty")
 	}
 
-	// check if name is already used
-	if _, ok := s.data[info.Name]; ok {
-		return fmt.Errorf("plugin name already in use %s", info.Name)
-	}
-
 	// image is a required field
 	if info.Image == "" {
 		return errors.New("plugin image cannot be empty")
@@ -564,6 +559,19 @@ func (s *registeredPlugins) add(baseMod BasePlugin, cliMod CommandLineMod, drMod
 		return ErrUnsupportedPlugin
 	}
 
+	isCandidatePluginYaml := yamlMod != nil
+	existingPlugin, alreadyPresent := s.data[info.Name]
+	if alreadyPresent {
+		if existingPlugin.IsYamlPlugin() == isCandidatePluginYaml {
+			return fmt.Errorf("plugin name already in use %s", info.Name)
+		}
+		// merge plugin case : existing plugin is binary and candidate is yaml
+		// (as binaries are loaded first)
+		s.data[info.Name].YamlMod = yamlMod
+		return nil
+	}
+
+	// creating new plugin
 	s.data[info.Name] = &Plugin{
 		Base:          baseMod,
 		CLIMod:        cliMod,

models/plugin_test.go

@@ -133,6 +133,7 @@ func TestPluginModels(t *testing.T) {
 			NewMockYamlPlugin("c", string(models.PluginTypeTask)),
 			NewMockYamlPlugin("b", string(models.PluginTypeTask)),
 			NewMockPlugin("a", string(models.PluginTypeHook)),
+			NewMockYamlPlugin("a", string(models.PluginTypeHook)),
 			NewMockYamlPlugin("z", string(models.PluginTypeTask)),
 		}
 		for _, plugin := range plugins {
@@ -142,6 +143,12 @@ func TestPluginModels(t *testing.T) {
 				repo.Add(plugin.Base, plugin.CLIMod, plugin.DependencyMod)
 			}
 		}
+		t.Run("should allow both yaml and bin implementations in plugin", func(t *testing.T) {
+			plugin, _ := repo.GetByName("a")
+			assert.Equal(t, plugin.IsYamlPlugin(), true)
+			assert.NotNil(t, plugin.CLIMod)
+			assert.NotNil(t, plugin.YamlMod)
+		})
 
 		t.Run("should return sorted plugins", func(t *testing.T) {
 			list := repo.GetAll()

plugin/plugin.go

@@ -109,13 +109,25 @@ func modSupported(mods []models.PluginMod, mod models.PluginMod) bool {
 func DiscoverPluginsGivenFilePattern(pluginLogger hclog.Logger, prefix, suffix string) []string {
 	var discoveredPlugins, dirs []string
 
-	// static plugin discovery in both server and client
 	if p, err := os.Getwd(); err == nil {
 		dirs = append(dirs, path.Join(p, PluginsDir))
+		dirs = append(dirs, p)
 	} else {
 		pluginLogger.Debug(fmt.Sprintf("Error discovering working dir: %s", err))
 	}
 
+	// look in the same directory as the executable
+	if exePath, err := os.Executable(); err != nil {
+		pluginLogger.Debug(fmt.Sprintf("Error discovering exe directory: %s", err))
+	} else {
+		dirs = append(dirs, filepath.Dir(exePath))
+	}
+
+	// add user home directory
+	if currentHomeDir, err := os.UserHomeDir(); err == nil {
+		dirs = append(dirs, filepath.Join(currentHomeDir, ".optimus", "plugins"))
+	}
+
 	for _, dirPath := range dirs {
 		fileInfos, err := os.ReadDir(dirPath)
 		if err != nil {

plugin/yaml/plugin.go

@@ -116,17 +116,13 @@ func NewPluginSpec(pluginPath string) (*PluginSpec, error) {
 // NOTE: binary plugins are loaded prior to yaml plugins
 func Init(pluginsRepo models.PluginRepository, discoveredYamlPlugins []string, pluginLogger hclog.Logger) {
 	for _, yamlPluginPath := range discoveredYamlPlugins {
-		yamlPlugin, err := NewPluginSpec(yamlPluginPath)
+		yamlPluginSpec, err := NewPluginSpec(yamlPluginPath)
 		if err != nil {
 			pluginLogger.Error(fmt.Sprintf("plugin Init: %s", yamlPluginPath), err)
 			continue
 		}
-		pluginInfo, _ := yamlPlugin.PluginInfo()
-		if plugin, _ := pluginsRepo.GetByName(pluginInfo.Name); plugin != nil && !plugin.IsYamlPlugin() {
-			pluginLogger.Debug(fmt.Sprintf("skipping yaml plugin (as binary already added): %s", pluginInfo.Name))
-			continue
-		}
-		if err := pluginsRepo.AddYaml(yamlPlugin); err != nil {
+		pluginInfo, _ := yamlPluginSpec.PluginInfo()
+		if err := pluginsRepo.AddYaml(yamlPluginSpec); err != nil {
 			pluginLogger.Error(fmt.Sprintf("PluginRegistry.Add: %s", yamlPluginPath), err)
 			continue
 		}

plugin/yaml/plugin_test.go

@@ -155,7 +155,7 @@ func TestYamlPlugin(t *testing.T) {
 			yaml.Init(repo, []string{testYamlPluginPath}, pluginLogger)
 			assert.NotEmpty(t, repo.GetAll())
 		})
-		t.Run("should not load yaml plugin due to binary plugin with same name exists", func(t *testing.T) {
+		t.Run("should load yaml even when binary plugin with same name exists", func(t *testing.T) {
 			repoWithBinayPlugin := models.NewPluginRepository()
 			err := repoWithBinayPlugin.Add(&mockBasePlugin{
 				Name:          testYamlPluginName,
@@ -171,6 +171,13 @@ func TestYamlPlugin(t *testing.T) {
 
 			assert.Len(t, repoPlugins, 1)
 			assert.Equal(t, repoPlugins[0].Info().Name, testYamlPluginName)
+			assert.NotNil(t, repoPlugins[0].YamlMod)
+		})
+		t.Run("should not load duplicate yaml", func(t *testing.T) {
+			repoWithBinayPlugin := models.NewPluginRepository()
+			yaml.Init(repoWithBinayPlugin, []string{testYamlPluginPath, testYamlPluginPath}, pluginLogger)
+			repoPlugins := repoWithBinayPlugin.GetAll()
+			assert.Len(t, repoPlugins, 1)
 		})
 		t.Run("should not load yaml plugin for invalid paths or yaml", func(t *testing.T) {
 			repo := models.NewPluginRepository()