From b9fda3ba3146fe3466c535f97ebcb6ce2939293a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Kosiec?= Date: Tue, 31 Jan 2023 17:02:13 +0100 Subject: [PATCH] Fix issues found during review --- internal/plugin/manager.go | 4 ++-- internal/plugin/manager_test.go | 2 +- pkg/config/config.go | 24 +++++++++---------- pkg/config/config_test.go | 4 ++-- pkg/config/plugin.go | 2 +- .../invalid-alias.yaml | 2 +- pkg/execute/alias.go | 4 ---- pkg/execute/alias_test.go | 8 +++---- pkg/execute/exec_test.go | 4 ++-- pkg/execute/kubectl.go | 4 ++-- pkg/execute/kubectl_cmd_builder.go | 4 +--- pkg/execute/source_test.go | 2 +- test/e2e/bots_test.go | 2 +- 13 files changed, 30 insertions(+), 36 deletions(-) diff --git a/internal/plugin/manager.go b/internal/plugin/manager.go index dad0c4e2f..37fc641c1 100644 --- a/internal/plugin/manager.go +++ b/internal/plugin/manager.go @@ -46,7 +46,7 @@ var pluginMap = map[string]plugin.Plugin{ type Manager struct { isStarted atomic.Bool log logrus.FieldLogger - cfg config.Plugins + cfg config.PluginManagement httpClient *http.Client executorsToEnable []string @@ -57,7 +57,7 @@ type Manager struct { } // NewManager returns a new Manager instance. -func NewManager(logger logrus.FieldLogger, cfg config.Plugins, executors, sources []string) *Manager { +func NewManager(logger logrus.FieldLogger, cfg config.PluginManagement, executors, sources []string) *Manager { return &Manager{ cfg: cfg, httpClient: newHTTPClient(), diff --git a/internal/plugin/manager_test.go b/internal/plugin/manager_test.go index 102ae9ac9..880d3ba69 100644 --- a/internal/plugin/manager_test.go +++ b/internal/plugin/manager_test.go @@ -69,7 +69,7 @@ func TestCollectEnabledRepositories(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // given - manager := NewManager(loggerx.NewNoop(), config.Plugins{ + manager := NewManager(loggerx.NewNoop(), config.PluginManagement{ Repositories: tc.definedRepositories, }, tc.enabledExecutors, tc.enabledSources) diff --git a/pkg/config/config.go b/pkg/config/config.go index 8d78fc1ab..3dc113bd2 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -136,15 +136,15 @@ type Config struct { Aliases Aliases `yaml:"aliases" validate:"dive"` Communications map[string]Communications `yaml:"communications" validate:"required,min=1,dive"` - Filters Filters `yaml:"filters"` - Analytics Analytics `yaml:"analytics"` - Settings Settings `yaml:"settings"` - ConfigWatcher CfgWatcher `yaml:"configWatcher"` - Plugins Plugins `yaml:"plugins"` + Filters Filters `yaml:"filters"` + Analytics Analytics `yaml:"analytics"` + Settings Settings `yaml:"settings"` + ConfigWatcher CfgWatcher `yaml:"configWatcher"` + Plugins PluginManagement `yaml:"plugins"` } -// Plugins holds Botkube plugins related configuration. -type Plugins struct { +// PluginManagement holds Botkube plugin management related configuration. +type PluginManagement struct { CacheDir string `yaml:"cacheDir"` Repositories map[string]PluginsRepositories `yaml:"repositories"` } @@ -210,7 +210,7 @@ type ActionBindings struct { type Sources struct { DisplayName string `yaml:"displayName"` Kubernetes KubernetesSource `yaml:"kubernetes"` - Plugins PluginsMap `koanf:",remain"` + Plugins Plugins `koanf:",remain"` } // KubernetesSource contains configuration for Kubernetes sources. @@ -290,8 +290,8 @@ type IngressRecommendations struct { TLSSecretValid *bool `yaml:"tlsSecretValid,omitempty"` } -// PluginsMap contains plugins configuration parameters defined in groups. -type PluginsMap map[string]Plugin +// Plugins contains plugins configuration parameters defined in groups. +type Plugins map[string]Plugin // Plugin contains plugin specific configuration. type Plugin struct { @@ -301,8 +301,8 @@ type Plugin struct { // Executors contains executors configuration parameters. type Executors struct { - Kubectl Kubectl `yaml:"kubectl"` - Plugins PluginsMap `koanf:",remain"` + Kubectl Kubectl `yaml:"kubectl"` + Plugins Plugins `koanf:",remain"` } // Aliases contains aliases configuration. diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 5c39506fd..9522a88bb 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -53,7 +53,7 @@ func TestLoadConfigSuccess(t *testing.T) { func TestLoadConfigWithPlugins(t *testing.T) { // given - expSourcePlugin := config.PluginsMap{ + expSourcePlugin := config.Plugins{ "botkube/keptn": { Enabled: true, Config: map[string]interface{}{ @@ -62,7 +62,7 @@ func TestLoadConfigWithPlugins(t *testing.T) { }, } - expExecutorPlugin := config.PluginsMap{ + expExecutorPlugin := config.Plugins{ "botkube/echo": { Enabled: true, Config: map[string]interface{}{ diff --git a/pkg/config/plugin.go b/pkg/config/plugin.go index ee9fe047d..21fdf574c 100644 --- a/pkg/config/plugin.go +++ b/pkg/config/plugin.go @@ -99,7 +99,7 @@ func validateBindPlugins(sl validator.StructLevel, enabledPluginsViaBindings []s } } -func validatePlugins(sl validator.StructLevel, pluginConfigs PluginsMap) { +func validatePlugins(sl validator.StructLevel, pluginConfigs Plugins) { var enabledPluginsViaBindings []string for pluginKey, plugin := range pluginConfigs { if !plugin.Enabled { diff --git a/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-alias.yaml b/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-alias.yaml index 9cce0115d..ae9bb8a5c 100644 --- a/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-alias.yaml +++ b/pkg/config/testdata/TestLoadedConfigValidationErrors/invalid-alias.yaml @@ -17,7 +17,7 @@ executors: 'kubectl-read-only': kubectl: enabled: true - helm: + 'helm': botkube/helm: config: {} enabled: true diff --git a/pkg/execute/alias.go b/pkg/execute/alias.go index 91ffd04ba..5e75a1bb6 100644 --- a/pkg/execute/alias.go +++ b/pkg/execute/alias.go @@ -103,10 +103,6 @@ func (e *AliasExecutor) getTabularOutput(bindings []string) string { fmt.Fprintf(w, "%s\t%s\t%s\n", aliasName, aliasCfg.Command, aliasCfg.DisplayName) } - if len(aliasesToDisplay) == 0 { - fmt.Fprintln(w, "No aliases found for current conversation.") - } - w.Flush() return buf.String() } diff --git a/pkg/execute/alias_test.go b/pkg/execute/alias_test.go index 7e14c75e5..d482ff2e8 100644 --- a/pkg/execute/alias_test.go +++ b/pkg/execute/alias_test.go @@ -82,7 +82,7 @@ func fixAliasCfg() config.Config { Kubectl: config.Kubectl{ Enabled: true, }, - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "gh": config.Plugin{ Enabled: false, }, @@ -92,14 +92,14 @@ func fixAliasCfg() config.Config { Kubectl: config.Kubectl{ Enabled: false, }, - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "gh": config.Plugin{ Enabled: true, }, }, }, "plugins": { - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "botkube/helm": config.Plugin{ Enabled: true, }, @@ -109,7 +109,7 @@ func fixAliasCfg() config.Config { }, }, "other": { - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "botkube/other@v1.0.1-devel": config.Plugin{ Enabled: true, }, diff --git a/pkg/execute/exec_test.go b/pkg/execute/exec_test.go index 47cb8ea13..ca92231af 100644 --- a/pkg/execute/exec_test.go +++ b/pkg/execute/exec_test.go @@ -76,14 +76,14 @@ func TestExecutorBindingsExecutor(t *testing.T) { }, }, "botkube/helm": { - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "botkube/helm": config.Plugin{ Enabled: true, }, }, }, "botkube/echo@v1.0.1-devel": { - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "botkube/echo@v1.0.1-devel": config.Plugin{ Enabled: true, }, diff --git a/pkg/execute/kubectl.go b/pkg/execute/kubectl.go index ca4b207b1..896d402b5 100644 --- a/pkg/execute/kubectl.go +++ b/pkg/execute/kubectl.go @@ -76,7 +76,7 @@ func (e *Kubectl) CanHandle(args []string) bool { // make sure that verb is also specified // empty `k|kc|kubectl` commands are handled by command builder - return len(args) >= 2 && args[0] == kubectlName + return len(args) >= 2 && args[0] == kubectlCommandName } // GetCommandPrefix gets verb command with k8s alias prefix. @@ -95,7 +95,7 @@ func (e *Kubectl) getArgsWithoutAlias(msg string) ([]string, error) { return nil, fmt.Errorf("while parsing the command message into args: %w", err) } - if len(msgParts) >= 2 && msgParts[0] == kubectlName { + if len(msgParts) >= 2 && msgParts[0] == kubectlCommandName { return msgParts[1:], nil } diff --git a/pkg/execute/kubectl_cmd_builder.go b/pkg/execute/kubectl_cmd_builder.go index 56f578ca1..cb236b866 100644 --- a/pkg/execute/kubectl_cmd_builder.go +++ b/pkg/execute/kubectl_cmd_builder.go @@ -38,8 +38,6 @@ var knownCmdPrefix = map[string]struct{}{ filterPlaintextInputCommand: {}, } -const kubectlName = "kubectl" - var errRequiredVerbDropdown = errors.New("verbs dropdown select cannot be empty") type ( @@ -89,7 +87,7 @@ func (e *KubectlCmdBuilder) GetCommandPrefix(args []string) string { case 1: // check if it's only a kubectl command without arguments - if args[0] == kubectlName { + if args[0] == kubectlCommandName { return args[0] } // it is a single arg which cannot start the kubectl command builder diff --git a/pkg/execute/source_test.go b/pkg/execute/source_test.go index a2939c941..2baecdaad 100644 --- a/pkg/execute/source_test.go +++ b/pkg/execute/source_test.go @@ -89,7 +89,7 @@ func TestSourceExecutor(t *testing.T) { }, "plugins": { DisplayName: "plugin-a", - Plugins: config.PluginsMap{ + Plugins: config.Plugins{ "plugin-a": { Enabled: true, }, diff --git a/test/e2e/bots_test.go b/test/e2e/bots_test.go index 286c1dcb4..64f294ad5 100644 --- a/test/e2e/bots_test.go +++ b/test/e2e/bots_test.go @@ -245,7 +245,7 @@ func runBotTest(t *testing.T, // Those are a temporary tests. When we will extract kubectl and kubernetes as plugins // they won't be needed anymore. - t.Run("Botkube Plugins", func(t *testing.T) { + t.Run("Botkube PluginManagement", func(t *testing.T) { t.Run("Echo Executor success", func(t *testing.T) { command := "echo test" expectedBody := codeBlock(strings.ToUpper(command))