WIP: plugins: add compile-time plugin system, migrate aws + ripeatlas

.github/workflows/test.yaml

@@ -56,12 +56,18 @@ jobs:
     name: Lint
     if: ${{ github.event_name == 'pull_request' }}
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        # golangci-lint works per module: lint every go.work workspace module
+        module: [".", "cmd/bot", "cmd/cli", "plugins/aws", "plugins/example", "plugins/ripeatlas"]
     steps:
       - uses: actions/checkout@v6
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v9.2.1
         with:
           only-new-issues: true
+          working-directory: ${{ matrix.module }}
           args: --timeout=3m
   security:
     name: Security
@@ -71,12 +77,13 @@ jobs:
       - name: Run Gosec Security Scanner
         uses: securego/gosec@master
         with:
-          args: -exclude=G404,G307,G104,G117,G705,G706 ./...
+          # use the full import path pattern to cover all go.work workspace modules
+          args: -exclude=G404,G307,G104,G117,G705,G706 github.com/innogames/slack-bot/v2/...
       - id: govulncheck
         uses: golang/govulncheck-action@v1
         with:
           repo-checkout: false
-          go-package: ./...
+          go-package: github.com/innogames/slack-bot/v2/...
   docker:
     name: Build Docker image
     runs-on: ubuntu-latest

.golangci.yml

@@ -1,4 +1,7 @@
 version: "2"
+run:
+  # make path based excludes work when linting the workspace modules (cmd/*, plugins/*)
+  relative-path-mode: cfg
 linters:
   enable:
     - asasalint
@@ -75,7 +78,7 @@ linters:
       # gosec taint analysis (XSS/log injection) does not apply.
       - linters:
           - gosec
-        path: (bot/tester/|command/clouds/)
+        path: (bot/tester/|plugins/aws/)
         text: (G705|G706)
       # pre-existing package names; renaming would churn every importer for no real benefit.
       - linters:

Makefile

@@ -5,6 +5,12 @@ all: test build/slack-bot build/cli
 
 FLAGS = -trimpath -ldflags="-s -w -X github.com/innogames/slack-bot/v2/bot/version.Version=$(shell git describe --tags)"
 
+# all packages of all go.work workspace modules: framework + cmd binaries + plugins
+PKGS = github.com/innogames/slack-bot/v2/...
+
+# all workspace modules, e.g. for per-module linting
+MODULE_DIRS = . ./cmd/bot ./cmd/cli ./plugins/aws ./plugins/example ./plugins/ripeatlas
+
 build/slack-bot: dep
 	@mkdir -p build/
 	go build $(FLAGS) -o build/slack-bot cmd/bot/main.go
@@ -26,13 +32,16 @@ run-cli-config:
 clean:
 	rm -rf build/
 
-# download go dependencies into ./vendor/
+# download go dependencies of all workspace modules into ./vendor/
 dep:
-	@go mod vendor
+	@go work vendor
 
 lint:
-	go fix ./...
-	golangci-lint run --fix
+	go fix $(PKGS)
+	@for dir in $(MODULE_DIRS); do \
+		echo "linting $$dir"; \
+		(cd $$dir && golangci-lint run --fix) || exit 1; \
+	done
 
 docker-build:
 	docker build . --force-rm -t brainexe/slack-bot:latest
@@ -41,17 +50,17 @@ docker-push:
 	docker push brainexe/slack-bot:latest
 
 test: dep
-	go test ./...
+	go test $(PKGS)
 
 test-race: dep
-	go test ./... -race
+	go test $(PKGS) -race
 
 test-bench:
-	go test -bench . ./... -benchmem
+	go test -bench . $(PKGS) -benchmem
 
 test-coverage: dep
 	@mkdir -p build
-	go test ./... -coverpkg=./... -cover -coverprofile=./build/cover.out -covermode=atomic
+	go test $(PKGS) -coverpkg=$(PKGS) -cover -coverprofile=./build/cover.out -covermode=atomic
 	go tool cover -html=./build/cover.out -o ./build/cover.html
 	@go tool cover -func ./build/cover.out | grep total | awk '{print "Total Coverage: " $$3 " see ./build/cover.html"}'
 

bot/config/config.go

@@ -27,6 +27,9 @@ type Config struct {
 		AccessToken string
 		Host        string
 	} `mapstructure:"gitlab"`
+	// Deprecated: the aws commands moved to plugins/aws and read their config
+	// via Config.LoadPlugin from the "plugins: aws:" section (the legacy
+	// top-level "aws:" key still works). Field kept for compatibility, unused at runtime.
 	Aws      Aws       `mapstructure:"aws"`
 	Commands []Command `mapstructure:"commands"`
 	Crons    []Cron    `mapstructure:"crons"`
@@ -41,9 +44,6 @@ type Config struct {
 	PullRequest PullRequest `mapstructure:"pullrequest"`
 	Timezone    string      `mapstructure:"timezone"`
 
-	// list of slack-bot plugins to load
-	Plugins []string `mapstructure:"plugins"`
-
 	// store whole Viper to get dynamic config values
 	viper *viper.Viper `mapstructure:"-"`
 }
@@ -59,7 +59,8 @@ func (c *Config) LoadCustom(key string, value any) error {
 // Set a dynamic config value...please only set it in tests!
 func (c *Config) Set(key string, value any) {
 	if c.viper == nil {
-		c.viper = viper.New()
+		// use the same key delimiter as the Load() based viper instance
+		c.viper = viper.NewWithOptions(viper.KeyDelimiter(keyDelimiter), viper.KeyPreserveCase())
 	}
 	c.viper.Set(key, value)
 }

bot/config/loader.go

@@ -10,10 +10,11 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
+// don't use '.' or '_' etc as delimiter, as it will block having this chars as map keys
+const keyDelimiter = "§"
+
 // Load all yaml config from a directory or a single .yaml file
 func Load(configFile string) (Config, error) {
-	// don't use '.' or '_' etc as delimiter, as it will block having this chars as map keys
-	keyDelimiter := "§"
 	v := viper.NewWithOptions(viper.KeyDelimiter(keyDelimiter), viper.KeyPreserveCase())
 
 	v.SetConfigType("yaml")

bot/config/plugin.go

@@ -0,0 +1,33 @@
+package config
+
+// pluginKey returns the viper key of a plugin's config section, e.g. "plugins§aws" for "plugins: aws:"
+func pluginKey(name string) string {
+	return "plugins" + keyDelimiter + name
+}
+
+// LoadPlugin unmarshals a plugin's config from the "plugins: <name>:" section of the config.
+// It falls back to the legacy top-level "<name>:" key (e.g. "aws:", "ripeatlas:")
+// when no "plugins: <name>:" section is defined.
+func (c *Config) LoadPlugin(name string, value any) error {
+	if c.viper == nil {
+		return nil
+	}
+	if c.viper.IsSet(pluginKey(name)) {
+		return c.viper.UnmarshalKey(pluginKey(name), value)
+	}
+	return c.viper.UnmarshalKey(name, value)
+}
+
+// IsPluginEnabled is the global plugin kill switch: only an explicit
+// "plugins: <name>: enabled: false" disables a plugin. A missing key means
+// enabled - plugins can still self-gate on their own config, e.g. on missing credentials.
+func (c *Config) IsPluginEnabled(name string) bool {
+	if c.viper == nil {
+		return true
+	}
+	key := pluginKey(name) + keyDelimiter + "enabled"
+	if !c.viper.IsSet(key) {
+		return true
+	}
+	return c.viper.GetBool(key)
+}

bot/config/plugin_test.go

@@ -0,0 +1,120 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type testPluginConfig struct {
+	Enabled bool   `mapstructure:"enabled"`
+	APIKey  string `mapstructure:"api_key"`
+}
+
+func TestLoadPlugin(t *testing.T) {
+	t.Run("nil viper", func(t *testing.T) {
+		cfg := Config{}
+
+		pluginCfg := testPluginConfig{APIKey: "default"}
+		require.NoError(t, cfg.LoadPlugin("my_plugin", &pluginCfg))
+
+		// defaults are untouched
+		assert.Equal(t, "default", pluginCfg.APIKey)
+	})
+
+	t.Run("load from plugins section", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("plugins§my_plugin", map[string]any{
+			"enabled": true,
+			"api_key": "secret",
+		})
+
+		pluginCfg := testPluginConfig{}
+		require.NoError(t, cfg.LoadPlugin("my_plugin", &pluginCfg))
+
+		assert.True(t, pluginCfg.Enabled)
+		assert.Equal(t, "secret", pluginCfg.APIKey)
+	})
+
+	t.Run("fallback to legacy top-level key", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("my_plugin", map[string]any{
+			"api_key": "legacy-secret",
+		})
+
+		pluginCfg := testPluginConfig{}
+		require.NoError(t, cfg.LoadPlugin("my_plugin", &pluginCfg))
+
+		assert.Equal(t, "legacy-secret", pluginCfg.APIKey)
+	})
+
+	t.Run("plugins section wins over legacy key", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("my_plugin", map[string]any{
+			"api_key": "legacy-secret",
+		})
+		cfg.Set("plugins§my_plugin", map[string]any{
+			"api_key": "new-secret",
+		})
+
+		pluginCfg := testPluginConfig{}
+		require.NoError(t, cfg.LoadPlugin("my_plugin", &pluginCfg))
+
+		assert.Equal(t, "new-secret", pluginCfg.APIKey)
+	})
+
+	t.Run("absent config keeps defaults", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("other_plugin", map[string]any{"api_key": "other"})
+
+		pluginCfg := testPluginConfig{APIKey: "default"}
+		require.NoError(t, cfg.LoadPlugin("my_plugin", &pluginCfg))
+
+		assert.Equal(t, "default", pluginCfg.APIKey)
+	})
+}
+
+func TestIsPluginEnabled(t *testing.T) {
+	t.Run("nil viper", func(t *testing.T) {
+		cfg := Config{}
+		assert.True(t, cfg.IsPluginEnabled("my_plugin"))
+	})
+
+	t.Run("no plugins section", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("something", "else")
+		assert.True(t, cfg.IsPluginEnabled("my_plugin"))
+	})
+
+	t.Run("section without enabled flag", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("plugins§my_plugin", map[string]any{"api_key": "secret"})
+		assert.True(t, cfg.IsPluginEnabled("my_plugin"))
+	})
+
+	t.Run("explicitly disabled", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("plugins§my_plugin", map[string]any{"enabled": false})
+		assert.False(t, cfg.IsPluginEnabled("my_plugin"))
+	})
+
+	t.Run("explicitly enabled", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Set("plugins§my_plugin", map[string]any{"enabled": true})
+		assert.True(t, cfg.IsPluginEnabled("my_plugin"))
+	})
+}
+
+// the whole plugin config design relies on nested key traversal (IsSet/UnmarshalKey
+// with the "§" delimiter) working in the viper fork - lock that behavior in
+func TestNestedViperKeys(t *testing.T) {
+	cfg := Config{}
+	cfg.Set("plugins§my_plugin", map[string]any{"enabled": false, "api_key": "secret"})
+
+	assert.True(t, cfg.viper.IsSet("plugins§my_plugin"))
+	assert.True(t, cfg.viper.IsSet("plugins§my_plugin§enabled"))
+	assert.False(t, cfg.viper.IsSet("plugins§other_plugin"))
+	assert.False(t, cfg.viper.GetBool("plugins§my_plugin§enabled"))
+	assert.Equal(t, "secret", cfg.viper.GetString("plugins§my_plugin§api_key"))
+}

bot/plugin.go

@@ -7,29 +7,43 @@ import (
 )
 
 // Plugin is an extended command which can be registered to the bot at compile time.
-// Plugin authors create a Go module with an init() function that calls RegisterPlugin().
-// Users activate plugins via blank imports: _ "github.com/company/my-plugin"
+// Plugin authors create a Go package with an init() function that calls RegisterPlugin().
+// Users activate plugins via blank imports in their main package,
+// e.g. _ "github.com/innogames/slack-bot/v2/plugins/all" or _ "github.com/company/my-plugin".
+// A compiled-in plugin can be disabled again with "plugins: <name>: enabled: false" in the config.
 type Plugin struct {
-	// Name is a unique identifier for the plugin, used in logging.
+	// Name is a unique identifier for the plugin, used in logging and as
+	// config key within the "plugins:" config section.
 	Name string
 
 	// Init creates the plugin's commands. It receives the SlackClient for Slack
 	// interaction and the Config for reading plugin-specific configuration
-	// via cfg.LoadCustom("my_plugin", &myConfig).
+	// via cfg.LoadPlugin("my_plugin", &myConfig).
 	Init func(slackClient client.SlackClient, cfg config.Config) Commands
 }
 
 var pluginList []Plugin
 
 // RegisterPlugin registers a new plugin, also in init() time
 func RegisterPlugin(plugin Plugin) {
+	for _, registered := range pluginList {
+		if registered.Name == plugin.Name {
+			log.Warnf("Plugin %s is registered multiple times", plugin.Name)
+		}
+	}
 	pluginList = append(pluginList, plugin)
 }
 
+// loadPlugins initializes all registered plugins which are not disabled via config.
+// It consumes the plugin list: a second call within the same process loads nothing.
 func loadPlugins(slackClient client.SlackClient, cfg config.Config) Commands {
 	commands := Commands{}
 
 	for _, plugin := range pluginList {
+		if !cfg.IsPluginEnabled(plugin.Name) {
+			log.Infof("Plugin %s is disabled via config", plugin.Name)
+			continue
+		}
 		log.Infof("Loading plugin: %s", plugin.Name)
 		commands.Merge(plugin.Init(slackClient, cfg))
 	}