From 10953bf96616ecd3e9892f0296a57928d454ae2f Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sat, 18 Apr 2026 12:32:13 +0200 Subject: [PATCH 01/10] feat(plugin): external command discovery (git/kubectl/gh style) On Execute(), scan $PATH for executables named stackctl- and register each as a top-level subcommand that proxies invocation to the external binary. First-path-wins semantics; built-in subcommands always win on name collision; non-regular files, directories, and non-executables are skipped; stdin/stdout/stderr and exit codes pass through. This unblocks third-party extensions without requiring stackctl itself to depend on any plugin SDK -- the only contract is the naming convention and environment-variable inheritance (STACKCTL_API_URL, STACKCTL_API_KEY, etc. are inherited naturally via os.Environ()). Tests (9 subtests) cover: discovery of valid plugins, skip non-executables, skip empty/missing $PATH entries, first-wins with duplicates, subcommand registration, built-in collision protection, argument passthrough, no-op on empty PATH, Cobra routing via RunE. Part of the broader extensibility refactor -- the Klaravik-specific refresh-db subcommand will move into a stackctl-refresh-db external plugin in a follow-up cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/cmd/plugins.go | 117 +++++++++++++++++++++++ cli/cmd/plugins_test.go | 199 ++++++++++++++++++++++++++++++++++++++++ cli/cmd/root.go | 5 + 3 files changed, 321 insertions(+) create mode 100644 cli/cmd/plugins.go create mode 100644 cli/cmd/plugins_test.go diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go new file mode 100644 index 0000000..0f5c996 --- /dev/null +++ b/cli/cmd/plugins.go @@ -0,0 +1,117 @@ +package cmd + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/spf13/cobra" +) + +// pluginPrefix is the filename prefix that marks an executable as a stackctl +// plugin. A binary at PATH/stackctl-foo becomes the subcommand `stackctl foo`. +const pluginPrefix = "stackctl-" + +// registerPlugins scans $PATH for executables named stackctl- and adds +// each as a top-level subcommand that proxies to the external binary. +// +// Behaviour: +// - Built-in subcommands always win on name collision; the plugin is skipped. +// - Earlier entries in $PATH win over later duplicates (standard PATH semantics). +// - Non-regular files, directories, and non-executables are ignored. +// - Plugin stdout/stderr/stdin pass through the parent tty; exit codes propagate. +// +// Pattern modelled on git, kubectl, and gh. See also: +// +// https://git-scm.com/docs/git#_git_commands +// https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/ +func registerPlugins(root *cobra.Command, pathEnv string) { + builtins := existingCommandNames(root) + for name, path := range discoverPlugins(pathEnv) { + if _, collides := builtins[name]; collides { + continue + } + root.AddCommand(newPluginCommand(name, path)) + builtins[name] = struct{}{} + } +} + +// discoverPlugins walks pathEnv (a colon-separated PATH) and returns a map +// of plugin name to absolute path. First-win semantics: earlier PATH entries +// take precedence over later ones when multiple binaries share a name. +func discoverPlugins(pathEnv string) map[string]string { + found := make(map[string]string) + for _, dir := range filepath.SplitList(pathEnv) { + if dir == "" { + continue + } + entries, err := os.ReadDir(dir) + if err != nil { + continue + } + for _, entry := range entries { + name := entry.Name() + if !strings.HasPrefix(name, pluginPrefix) || name == pluginPrefix { + continue + } + pluginName := strings.TrimPrefix(name, pluginPrefix) + if _, seen := found[pluginName]; seen { + continue + } + full := filepath.Join(dir, name) + info, err := os.Stat(full) + if err != nil || !info.Mode().IsRegular() { + continue + } + if info.Mode().Perm()&0o111 == 0 { + continue + } + found[pluginName] = full + } + } + return found +} + +// existingCommandNames returns the set of top-level subcommand names already +// registered on root, so discovery can avoid clobbering built-ins. +func existingCommandNames(root *cobra.Command) map[string]struct{} { + names := make(map[string]struct{}) + for _, c := range root.Commands() { + names[c.Name()] = struct{}{} + for _, alias := range c.Aliases { + names[alias] = struct{}{} + } + } + return names +} + +// newPluginCommand wraps an external binary as a Cobra subcommand. The binary +// receives all arguments after the plugin name; stdin/stdout/stderr pass +// through directly, and the exit code propagates to the caller. +func newPluginCommand(name, binaryPath string) *cobra.Command { + return &cobra.Command{ + Use: name, + Short: "Plugin: " + name + " (" + binaryPath + ")", + DisableFlagParsing: true, + SilenceUsage: true, + SilenceErrors: true, + RunE: func(cmd *cobra.Command, args []string) error { + proc := exec.Command(binaryPath, args...) //nolint:gosec // binary path discovered from PATH, not user input + proc.Stdin = cmd.InOrStdin() + proc.Stdout = cmd.OutOrStdout() + proc.Stderr = cmd.ErrOrStderr() + proc.Env = os.Environ() + if err := proc.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + // Propagate the plugin's exit code. Cobra will surface + // the error; explicit os.Exit keeps the code intact. + os.Exit(exitErr.ExitCode()) + } + return fmt.Errorf("plugin %q: %w", name, err) + } + return nil + }, + } +} diff --git a/cli/cmd/plugins_test.go b/cli/cmd/plugins_test.go new file mode 100644 index 0000000..f6beffc --- /dev/null +++ b/cli/cmd/plugins_test.go @@ -0,0 +1,199 @@ +package cmd + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writeScript writes a shell script executable at dir/name with the given body. +// Returns the absolute path. Skips the test on Windows — shell scripts and 0o755 +// aren't a useful way to exercise plugin discovery there. +func writeScript(t *testing.T, dir, name, body string) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("plugin discovery tests use shell scripts; skip on Windows") + } + path := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(path, []byte(body), 0o755)) + return path +} + +func TestDiscoverPlugins_FindsExecutableStackctlBinaries(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-hello", "#!/bin/sh\necho hi\n") + // Also place a non-plugin binary that shouldn't match. + _ = writeScript(t, dir, "notaplugin", "#!/bin/sh\necho nope\n") + // And a bare "stackctl-" which is the empty-name case — must be ignored. + _ = writeScript(t, dir, "stackctl-", "#!/bin/sh\necho empty\n") + + got := discoverPlugins(dir) + require.Contains(t, got, "hello") + assert.Len(t, got, 1) + assert.True(t, filepath.IsAbs(got["hello"])) +} + +func TestDiscoverPlugins_SkipsNonExecutable(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + nonExec := filepath.Join(dir, "stackctl-readonly") + require.NoError(t, os.WriteFile(nonExec, []byte("#!/bin/sh\necho hi\n"), 0o644)) + + got := discoverPlugins(dir) + assert.NotContains(t, got, "readonly", "non-executable files must be ignored") +} + +func TestDiscoverPlugins_FirstPathEntryWins(t *testing.T) { + t.Parallel() + + dir1 := t.TempDir() + dir2 := t.TempDir() + _ = writeScript(t, dir1, "stackctl-same", "#!/bin/sh\necho first\n") + _ = writeScript(t, dir2, "stackctl-same", "#!/bin/sh\necho second\n") + + got := discoverPlugins(dir1 + string(os.PathListSeparator) + dir2) + require.Contains(t, got, "same") + assert.Equal(t, filepath.Join(dir1, "stackctl-same"), got["same"]) +} + +func TestDiscoverPlugins_IgnoresMissingAndEmptyPATHEntries(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-ok", "#!/bin/sh\necho ok\n") + + got := discoverPlugins(string(os.PathListSeparator) + "/nonexistent/path" + string(os.PathListSeparator) + dir) + assert.Contains(t, got, "ok") +} + +func TestRegisterPlugins_AddsPluginAsSubcommand(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-greet", "#!/bin/sh\necho hello-from-plugin\n") + + root := &cobra.Command{Use: "stackctl"} + builtin := &cobra.Command{Use: "config", Short: "builtin"} + root.AddCommand(builtin) + + registerPlugins(root, dir) + + var greet *cobra.Command + for _, c := range root.Commands() { + if c.Name() == "greet" { + greet = c + break + } + } + require.NotNil(t, greet, "plugin subcommand must be registered") + assert.Contains(t, greet.Short, "Plugin:") +} + +func TestRegisterPlugins_BuiltinWinsOnCollision(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-config", "#!/bin/sh\necho shadow\n") + + root := &cobra.Command{Use: "stackctl"} + builtin := &cobra.Command{Use: "config", Short: "builtin"} + root.AddCommand(builtin) + + registerPlugins(root, dir) + + var found *cobra.Command + for _, c := range root.Commands() { + if c.Name() == "config" { + found = c + break + } + } + require.NotNil(t, found) + assert.Equal(t, "builtin", found.Short, "built-in must not be replaced by a colliding plugin") +} + +func TestRegisterPlugins_PluginInvocationPassesThroughArgsAndStdout(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-echo", + "#!/bin/sh\nprintf '%s|' \"$@\"\n") + + root := &cobra.Command{Use: "stackctl"} + registerPlugins(root, dir) + + var pluginCmd *cobra.Command + for _, c := range root.Commands() { + if c.Name() == "echo" { + pluginCmd = c + break + } + } + require.NotNil(t, pluginCmd) + + // The plugin uses os.Exit on error; on success we get its stdout. + // Cobra's Execute path would os.Exit; drive the wrapped binary directly via + // exec for a clean, assertable invocation. + bin := filepath.Join(dir, "stackctl-echo") + out, err := exec.Command(bin, "alpha", "--flag=beta", "gamma").CombinedOutput() + require.NoError(t, err) + assert.Equal(t, "alpha|--flag=beta|gamma|", string(out)) +} + +func TestRegisterPlugins_NoOpOnEmptyPath(t *testing.T) { + t.Parallel() + + root := &cobra.Command{Use: "stackctl"} + before := len(root.Commands()) + registerPlugins(root, "") + assert.Equal(t, before, len(root.Commands())) +} + +// TestRegisterPlugins_RunViaCobraRouting wires a plugin through Cobra's +// Execute path to verify the command is actually routed by name. The plugin +// exits 0, so the parent survives; args are captured via a sentinel file +// the plugin writes. +func TestRegisterPlugins_RunViaCobraRouting(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + sentinel := filepath.Join(t.TempDir(), "args.txt") + script := "#!/bin/sh\nprintf '%s ' \"$@\" > " + sentinel + "\n" + _ = writeScript(t, dir, "stackctl-touch", script) + + root := &cobra.Command{Use: "stackctl", SilenceUsage: true, SilenceErrors: true} + registerPlugins(root, dir) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"touch", "--hello=world", "posarg"}) + + // Run the plugin directly via the command's RunE rather than via Execute — + // Execute may call os.Exit(0) on failure paths via our plugin wrapper and + // that would terminate the test process. + var touchCmd *cobra.Command + for _, c := range root.Commands() { + if c.Name() == "touch" { + touchCmd = c + break + } + } + require.NotNil(t, touchCmd) + require.NoError(t, touchCmd.RunE(touchCmd, []string{"--hello=world", "posarg"})) + + contents, err := os.ReadFile(sentinel) + require.NoError(t, err) + assert.Contains(t, string(contents), "--hello=world") + assert.Contains(t, string(contents), "posarg") +} diff --git a/cli/cmd/root.go b/cli/cmd/root.go index 1d53153..17b6d2b 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -86,6 +86,11 @@ func init() { // Execute runs the root command. func Execute() error { + // Discover external plugins from $PATH and register any that do not + // collide with built-in commands. Ignoring collisions rather than + // erroring keeps the CLI usable if a user happens to have a + // `stackctl-` binary lying around. + registerPlugins(rootCmd, os.Getenv("PATH")) return rootCmd.Execute() } From 099614ab7ad27ce425857bfd08ccb7b7d9d26041 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sat, 18 Apr 2026 12:37:01 +0200 Subject: [PATCH 02/10] feat(output): pluggable format registry in pkg/output Users can now add custom output formats via RegisterFormat(name, fn) alongside the built-in table/json/yaml. A matching RegisterSingleFormat adds custom single-item rendering; when absent, single items adapt to the list formatter via a 2-column (field, value) table. Built-in format names cannot be shadowed (panics to surface the mistake at init). Unknown format names fall back to table, matching existing behaviour for users who misspell --output flags. Mirrors the extension pattern the backend uses for template funcs: global registry, register at init time, no mutation during concurrent render. Tests (8) cover: panic on built-in override, NewPrinter recognises custom format, unknown format falls back to table, list formatter receives data and headers/rows, single-item fallback adapts to list formatter, custom single formatter wins when registered, concurrent reads are race-free. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/pkg/output/formatter_test.go | 155 +++++++++++++++++++++++++++++++ cli/pkg/output/output.go | 94 ++++++++++++++++++- 2 files changed, 247 insertions(+), 2 deletions(-) create mode 100644 cli/pkg/output/formatter_test.go diff --git a/cli/pkg/output/formatter_test.go b/cli/pkg/output/formatter_test.go new file mode 100644 index 0000000..63c5556 --- /dev/null +++ b/cli/pkg/output/formatter_test.go @@ -0,0 +1,155 @@ +package output + +import ( + "bytes" + "fmt" + "io" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// registry state bleeds across tests since it's package-global. tests that +// register formats clean up via t.Cleanup. +func unregisterForTest(t *testing.T, names ...string) { + t.Helper() + t.Cleanup(func() { + formatRegistryMu.Lock() + defer formatRegistryMu.Unlock() + for _, n := range names { + delete(formatRegistry, n) + delete(singleRegistry, n) + } + }) +} + +func TestRegisterFormat_PanicOnBuiltinOverride(t *testing.T) { + t.Parallel() + for _, name := range []string{"table", "json", "yaml"} { + name := name + t.Run(name, func(t *testing.T) { + t.Parallel() + assert.PanicsWithValue(t, + fmt.Sprintf("output: cannot override built-in format %q", name), + func() { RegisterFormat(name, nil) }) + }) + } +} + +func TestNewPrinter_UsesCustomFormat(t *testing.T) { + t.Parallel() + RegisterFormat("csv", func(w io.Writer, _ interface{}, headers []string, rows [][]string) error { + _, err := fmt.Fprintln(w, strings.Join(headers, ",")) + if err != nil { + return err + } + for _, r := range rows { + if _, err := fmt.Fprintln(w, strings.Join(r, ",")); err != nil { + return err + } + } + return nil + }) + unregisterForTest(t, "csv") + + var buf bytes.Buffer + p := NewPrinter("csv", false, true) + p.Writer = &buf + require.Equal(t, Format("csv"), p.Format) + + err := p.Print(nil, []string{"id", "name"}, [][]string{{"1", "alpha"}, {"2", "beta"}}, []string{"1", "2"}) + require.NoError(t, err) + assert.Equal(t, "id,name\n1,alpha\n2,beta\n", buf.String()) +} + +func TestNewPrinter_UnknownFormatFallsBackToTable(t *testing.T) { + t.Parallel() + p := NewPrinter("neverregistered", false, true) + assert.Equal(t, FormatTable, p.Format) +} + +func TestPrint_CustomFormatReceivesTableArgs(t *testing.T) { + t.Parallel() + + var ( + gotHeaders []string + gotRows [][]string + gotData interface{} + ) + RegisterFormat("capture", func(_ io.Writer, data interface{}, headers []string, rows [][]string) error { + gotData = data + gotHeaders = headers + gotRows = rows + return nil + }) + unregisterForTest(t, "capture") + + p := NewPrinter("capture", false, true) + p.Writer = io.Discard + err := p.Print("payload", []string{"h1"}, [][]string{{"r1"}}, nil) + require.NoError(t, err) + assert.Equal(t, "payload", gotData) + assert.Equal(t, []string{"h1"}, gotHeaders) + assert.Equal(t, [][]string{{"r1"}}, gotRows) +} + +func TestPrintSingle_FallsBackToListFormatterWhenNoSingleRegistered(t *testing.T) { + t.Parallel() + + var ( + gotRows [][]string + ) + RegisterFormat("csv2", func(_ io.Writer, _ interface{}, _ []string, rows [][]string) error { + gotRows = rows + return nil + }) + unregisterForTest(t, "csv2") + + p := NewPrinter("csv2", false, true) + p.Writer = io.Discard + require.NoError(t, p.PrintSingle(nil, []KeyValue{{Key: "id", Value: "42"}, {Key: "name", Value: "x"}})) + + assert.Equal(t, [][]string{{"id", "42"}, {"name", "x"}}, gotRows) +} + +func TestRegisterSingleFormat_UsedWhenPresent(t *testing.T) { + t.Parallel() + + var called bool + RegisterFormat("custom1", func(io.Writer, interface{}, []string, [][]string) error { return nil }) + RegisterSingleFormat("custom1", func(_ io.Writer, _ interface{}, fields []KeyValue) error { + called = true + assert.Len(t, fields, 2) + return nil + }) + unregisterForTest(t, "custom1") + + p := NewPrinter("custom1", false, true) + p.Writer = io.Discard + require.NoError(t, p.PrintSingle(nil, []KeyValue{{Key: "a", Value: "1"}, {Key: "b", Value: "2"}})) + assert.True(t, called, "single formatter takes precedence over list formatter when registered") +} + +// TestRegisterFormat_ConcurrentReadsWithoutRegistration ensures that the +// read path is safe when no writers are active. Combined reads/writes are +// not supported — documented as "register at init time". +func TestRegisterFormat_ConcurrentReadsWithoutRegistration(t *testing.T) { + t.Parallel() + RegisterFormat("concurrent", func(io.Writer, interface{}, []string, [][]string) error { return nil }) + unregisterForTest(t, "concurrent") + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 50; j++ { + _, _ = lookupFormat("concurrent") + } + }() + } + wg.Wait() +} diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index 0a66376..bbbacb9 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -6,6 +6,7 @@ import ( "io" "os" "strings" + "sync" "text/tabwriter" "gopkg.in/yaml.v3" @@ -20,6 +21,61 @@ const ( FormatYAML Format = "yaml" ) +// FormatterFunc renders data in a custom format to w. The headers/rows +// pair is populated for list-style output; data is the same payload for +// a structured format like JSON. Implementations may use either or both. +type FormatterFunc func(w io.Writer, data interface{}, headers []string, rows [][]string) error + +// SingleFormatterFunc renders a single item's key/value fields. Optional; +// when a custom format does not register a single formatter, PrintSingle +// falls back to the list formatter with headers=["field", "value"]. +type SingleFormatterFunc func(w io.Writer, data interface{}, fields []KeyValue) error + +var ( + formatRegistryMu sync.RWMutex + formatRegistry = map[string]FormatterFunc{} + singleRegistry = map[string]SingleFormatterFunc{} +) + +// RegisterFormat attaches a named custom format. Call at init time, before +// any Printer is used concurrently — the registry is read-locked on every +// render so adds during rendering are racy. +// +// Name must not collide with the built-in formats (table, json, yaml). +// Passing a built-in name panics to surface the mistake early. +func RegisterFormat(name string, fn FormatterFunc) { + if name == string(FormatTable) || name == string(FormatJSON) || name == string(FormatYAML) { + panic(fmt.Sprintf("output: cannot override built-in format %q", name)) + } + formatRegistryMu.Lock() + defer formatRegistryMu.Unlock() + formatRegistry[name] = fn +} + +// RegisterSingleFormat attaches a custom single-item formatter for an +// already-registered format. Optional — only needed when the list and +// single shapes genuinely differ. +func RegisterSingleFormat(name string, fn SingleFormatterFunc) { + formatRegistryMu.Lock() + defer formatRegistryMu.Unlock() + singleRegistry[name] = fn +} + +// lookupFormat returns the registered list formatter for name. +func lookupFormat(name string) (FormatterFunc, bool) { + formatRegistryMu.RLock() + defer formatRegistryMu.RUnlock() + fn, ok := formatRegistry[name] + return fn, ok +} + +func lookupSingleFormat(name string) (SingleFormatterFunc, bool) { + formatRegistryMu.RLock() + defer formatRegistryMu.RUnlock() + fn, ok := singleRegistry[name] + return fn, ok +} + // Printer handles formatted output. type Printer struct { Format Format @@ -29,13 +85,24 @@ type Printer struct { } // NewPrinter creates a new Printer with the given format. +// Falls back to FormatTable if the name is empty, unknown, or a misspelt +// built-in. Custom formats registered via RegisterFormat are recognised. func NewPrinter(format string, quiet, noColor bool) *Printer { - f := FormatTable - switch strings.ToLower(format) { + name := strings.ToLower(format) + var f Format + switch name { + case "", "table": + f = FormatTable case "json": f = FormatJSON case "yaml": f = FormatYAML + default: + if _, ok := lookupFormat(name); ok { + f = Format(name) + } else { + f = FormatTable + } } return &Printer{ Format: f, @@ -125,7 +192,12 @@ func (p *Printer) Print(data interface{}, headers []string, rows [][]string, ids return p.PrintJSON(data) case FormatYAML: return p.PrintYAML(data) + case FormatTable: + return p.PrintTable(headers, rows) default: + if fn, ok := lookupFormat(string(p.Format)); ok { + return fn(p.Writer, data, headers, rows) + } return p.PrintTable(headers, rows) } } @@ -138,7 +210,25 @@ func (p *Printer) PrintSingle(data interface{}, fields []KeyValue) error { return p.PrintJSON(data) case FormatYAML: return p.PrintYAML(data) + case FormatTable: + w := p.TableWriter() + for _, f := range fields { + fmt.Fprintf(w, "%s:\t%s\n", f.Key, f.Value) + } + return w.Flush() default: + // Custom single formatter wins; otherwise adapt the list formatter by + // synthesising a two-column (field, value) table. + if fn, ok := lookupSingleFormat(string(p.Format)); ok { + return fn(p.Writer, data, fields) + } + if fn, ok := lookupFormat(string(p.Format)); ok { + rows := make([][]string, len(fields)) + for i, f := range fields { + rows[i] = []string{f.Key, f.Value} + } + return fn(p.Writer, data, []string{"field", "value"}, rows) + } w := p.TableWriter() for _, f := range fields { fmt.Fprintf(w, "%s:\t%s\n", f.Key, f.Value) From 507689f4734ff766fe4c11daa0ec4de51d3e23a5 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 09:30:33 +0200 Subject: [PATCH 03/10] =?UTF-8?q?feat(cli)!:=20remove=20`stack=20refresh-d?= =?UTF-8?q?b`=20=E2=80=94=20migrated=20to=20external=20plugin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parity has been verified end-to-end in the dev cluster (commit 4d82515 on k8s-stack-manager removed the corresponding backend endpoint). The refresh-db operation now lives entirely in the stackctl-refresh-db plugin shipped from kvk-devops-tools, discovered via the PATH-scanning plugin mechanism added in 10953bf. Removed: - stackRefreshDBCmd in cmd/stack.go (definition + flag + subcommand registration) - Client.RefreshDBStack in pkg/client/client.go - TestStackRefreshDBCmd_* (4 tests) in cmd/stack_test.go - TestRefreshDBStack_Success + _Conflict in pkg/client/client_test.go BREAKING CHANGE: `stackctl stack refresh-db ` no longer exists. Install the stackctl-refresh-db plugin from kvk-devops-tools/refresh-db/ onto your PATH; `stackctl refresh-db ` then becomes available automatically via external-command discovery. The top-level name changed too (refresh-db is no longer a stack-scoped subcommand). Full test suite green across all 6 packages. --- cli/cmd/stack.go | 63 -------------------- cli/cmd/stack_test.go | 106 ---------------------------------- cli/pkg/client/client.go | 14 ----- cli/pkg/client/client_test.go | 40 ------------- 4 files changed, 223 deletions(-) diff --git a/cli/cmd/stack.go b/cli/cmd/stack.go index 300e0cd..2d750ea 100644 --- a/cli/cmd/stack.go +++ b/cli/cmd/stack.go @@ -301,65 +301,6 @@ Examples: }, } -var stackRefreshDBCmd = &cobra.Command{ - Use: "refresh-db ", - Short: "Refresh a stack's MySQL database from the golden-db snapshot", - Long: `Refresh the MySQL database for a running stack instance without a full -clean+redeploy. Wipes the MySQL PVC so the init container re-extracts the -golden-db snapshot on next boot, flushes Redis, and deletes the storefront -sync Job so the next ` + "`stack deploy`" + ` re-fires the Helm hook that repopulates -Redis. - -Does NOT re-run helm — only scales deployments and runs a short-lived -cleanup Job. The stack must be in 'running' state. - -This is a destructive operation (wipes all MySQL data for this stack). -You will be prompted for confirmation unless --yes is specified. - -After refresh-db completes, run ` + "`stack deploy`" + ` to re-populate Redis via -the sync Job. - -Examples: - stackctl stack refresh-db 42 - stackctl stack refresh-db 42 --yes`, - Args: cobra.ExactArgs(1), - SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { - id, err := parseID(args[0]) - if err != nil { - return err - } - - confirmed, err := confirmAction(cmd, fmt.Sprintf("This will wipe MySQL data for stack %s and reload from golden-db. Continue? (y/n): ", id)) - if err != nil { - return err - } - if !confirmed { - printer.PrintMessage("Aborted.") - return nil - } - - c, err := newClient() - if err != nil { - return err - } - - log, err := c.RefreshDBStack(id) - if err != nil { - return err - } - - if printer.Quiet { - fmt.Fprintln(printer.Writer, log.ID) - return nil - } - - printer.PrintMessage("Refreshing database for stack %s... (log ID: %s)", id, log.ID) - printer.PrintMessage("Run 'stackctl stack logs %s' to check progress, or 'stackctl stack deploy %s' afterwards to re-populate Redis.", id, id) - return nil - }, -} - var stackDeleteCmd = &cobra.Command{ Use: "delete ", Short: "Delete a stack instance", @@ -709,9 +650,6 @@ func init() { // stack clean flags stackCleanCmd.Flags().BoolP("yes", "y", false, "Skip confirmation prompt") - // stack refresh-db flags - stackRefreshDBCmd.Flags().BoolP("yes", "y", false, "Skip confirmation prompt") - // stack delete flags stackDeleteCmd.Flags().BoolP("yes", "y", false, "Skip confirmation prompt") @@ -729,7 +667,6 @@ func init() { stackCmd.AddCommand(stackDeployCmd) stackCmd.AddCommand(stackStopCmd) stackCmd.AddCommand(stackCleanCmd) - stackCmd.AddCommand(stackRefreshDBCmd) stackCmd.AddCommand(stackDeleteCmd) stackCmd.AddCommand(stackStatusCmd) stackCmd.AddCommand(stackLogsCmd) diff --git a/cli/cmd/stack_test.go b/cli/cmd/stack_test.go index 1be5376..c190971 100644 --- a/cli/cmd/stack_test.go +++ b/cli/cmd/stack_test.go @@ -489,112 +489,6 @@ func TestStackCleanCmd_WithYesFlag(t *testing.T) { assert.Contains(t, buf.String(), "Cleaning stack 42") } -// ---------- stack refresh-db (destructive) ---------- - -func TestStackRefreshDBCmd_WithConfirmation(t *testing.T) { - called := false - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - called = true - require.Equal(t, http.MethodPost, r.Method) - require.Equal(t, "/api/v1/stack-instances/42/refresh-db", r.URL.Path) - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(types.DeploymentLog{Base: types.Base{ID: "log-104"}}) - })) - defer server.Close() - - buf := setupStackTestCmd(t, server.URL) - - stackRefreshDBCmd.Flags().Set("yes", "false") - t.Cleanup(func() { - stackRefreshDBCmd.Flags().Set("yes", "false") - stackRefreshDBCmd.SetIn(nil) - stackRefreshDBCmd.SetErr(nil) - }) - - stackRefreshDBCmd.SetIn(strings.NewReader("y\n")) - stackRefreshDBCmd.SetErr(&bytes.Buffer{}) - - err := stackRefreshDBCmd.RunE(stackRefreshDBCmd, []string{"42"}) - require.NoError(t, err) - assert.True(t, called, "API should be called after confirming with y") - assert.Contains(t, buf.String(), "Refreshing database for stack 42") - assert.Contains(t, buf.String(), "log-104") -} - -func TestStackRefreshDBCmd_Declined(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - t.Fatal("API should NOT be called when user declines") - })) - defer server.Close() - - buf := setupStackTestCmd(t, server.URL) - - stackRefreshDBCmd.Flags().Set("yes", "false") - t.Cleanup(func() { - stackRefreshDBCmd.Flags().Set("yes", "false") - stackRefreshDBCmd.SetIn(nil) - stackRefreshDBCmd.SetErr(nil) - }) - - stackRefreshDBCmd.SetIn(strings.NewReader("n\n")) - stackRefreshDBCmd.SetErr(&bytes.Buffer{}) - - err := stackRefreshDBCmd.RunE(stackRefreshDBCmd, []string{"42"}) - require.NoError(t, err) - assert.Contains(t, buf.String(), "Aborted") -} - -func TestStackRefreshDBCmd_WithYesFlag(t *testing.T) { - called := false - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - called = true - require.Equal(t, http.MethodPost, r.Method) - require.Equal(t, "/api/v1/stack-instances/42/refresh-db", r.URL.Path) - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(types.DeploymentLog{Base: types.Base{ID: "log-105"}}) - })) - defer server.Close() - - buf := setupStackTestCmd(t, server.URL) - - stackRefreshDBCmd.Flags().Set("yes", "true") - t.Cleanup(func() { stackRefreshDBCmd.Flags().Set("yes", "false") }) - - err := stackRefreshDBCmd.RunE(stackRefreshDBCmd, []string{"42"}) - require.NoError(t, err) - assert.True(t, called, "API should be called with --yes flag") - assert.Contains(t, buf.String(), "Refreshing database for stack 42") -} - -func TestStackRefreshDBCmd_QuietPrintsOnlyLogID(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(types.DeploymentLog{Base: types.Base{ID: "log-quiet-106"}}) - })) - defer server.Close() - - buf := setupStackTestCmd(t, server.URL) - - stackRefreshDBCmd.Flags().Set("yes", "true") - prevQuiet := printer.Quiet - printer.Quiet = true - t.Cleanup(func() { - stackRefreshDBCmd.Flags().Set("yes", "false") - printer.Quiet = prevQuiet - }) - - err := stackRefreshDBCmd.RunE(stackRefreshDBCmd, []string{"42"}) - require.NoError(t, err) - out := buf.String() - assert.Contains(t, out, "log-quiet-106") - // Quiet mode must not print the human-readable prefix. - assert.NotContains(t, out, "Refreshing database for stack") - assert.NotContains(t, out, "Run 'stackctl stack logs") -} - // ---------- stack delete (destructive) ---------- func TestStackDeleteCmd_WithConfirmation(t *testing.T) { diff --git a/cli/pkg/client/client.go b/cli/pkg/client/client.go index 43e330d..93b4dfa 100644 --- a/cli/pkg/client/client.go +++ b/cli/pkg/client/client.go @@ -330,20 +330,6 @@ func (c *Client) CleanStack(id string) (*types.DeploymentLog, error) { return &log, nil } -// RefreshDBStack triggers a database refresh for a stack instance: wipes the -// MySQL PVC so the init container re-extracts the golden-db snapshot on next -// boot, flushes Redis, and deletes the storefront sync Job so the next deploy -// re-fires the Helm hook. Does NOT re-run helm — only orchestrates k8s -// primitives. Status must be `running`. -func (c *Client) RefreshDBStack(id string) (*types.DeploymentLog, error) { - var log types.DeploymentLog - err := c.Post(fmt.Sprintf("/api/v1/stack-instances/%s/refresh-db", id), nil, &log) - if err != nil { - return nil, err - } - return &log, nil -} - // GetStackStatus returns the current status and pod states for a stack instance. func (c *Client) GetStackStatus(id string) (*types.InstanceStatus, error) { var status types.InstanceStatus diff --git a/cli/pkg/client/client_test.go b/cli/pkg/client/client_test.go index 529bbb0..8c8a238 100644 --- a/cli/pkg/client/client_test.go +++ b/cli/pkg/client/client_test.go @@ -769,46 +769,6 @@ func TestCleanStack_Success(t *testing.T) { assert.Equal(t, "clean", log.Action) } -func TestRefreshDBStack_Success(t *testing.T) { - t.Parallel() - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, http.MethodPost, r.Method) - assert.Equal(t, "/api/v1/stack-instances/42/refresh-db", r.URL.Path) - w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(types.DeploymentLog{ - Base: types.Base{ID: "log-103"}, - InstanceID: "42", - Action: "refresh-db", - Status: "started", - }) - })) - defer server.Close() - - c := New(server.URL) - log, err := c.RefreshDBStack("42") - require.NoError(t, err) - assert.Equal(t, "log-103", log.ID) - assert.Equal(t, "refresh-db", log.Action) -} - -func TestRefreshDBStack_Conflict(t *testing.T) { - t.Parallel() - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusConflict) - json.NewEncoder(w).Encode(map[string]string{"error": "Cannot refresh-db: instance is currently deploying"}) - })) - defer server.Close() - - c := New(server.URL) - log, err := c.RefreshDBStack("42") - require.Error(t, err) - assert.Nil(t, log) - - apiErr, ok := err.(*APIError) - require.True(t, ok, "expected APIError, got %T", err) - assert.Equal(t, http.StatusConflict, apiErr.StatusCode) -} - func TestGetStackStatus_Success(t *testing.T) { t.Parallel() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From c43ccb98dbf0937cdedc89c336e59bf02535624b Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 10:02:01 +0200 Subject: [PATCH 04/10] docs(plugins): add EXTENDING.md guide for stackctl plugin authors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes external-command discovery (10953bf) as a headline feature with tutorial-first documentation. - EXTENDING.md (repo root, ~280 lines): 5-minute tutorial (stackctl-hello in 4 lines of bash), use-case guidance, naming convention, environment inheritance, three language recipes (bash / Python / Go) for the most common pattern (invoke a server-side action), best practices, plugin distribution strategies, internals overview, troubleshooting. - README.md: "Extending — add your own subcommands" section right after Quick Start, with a copy-paste example and a link to EXTENDING.md. Cross-links to k8s-stack-manager's EXTENDING.md — the plugin side and the webhook side together form the complete end-to-end story for adding custom operations without forking either tool. --- EXTENDING.md | 380 +++++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 24 ++++ 2 files changed, 404 insertions(+) create mode 100644 EXTENDING.md diff --git a/EXTENDING.md b/EXTENDING.md new file mode 100644 index 0000000..f6094cc --- /dev/null +++ b/EXTENDING.md @@ -0,0 +1,380 @@ +# Extending stackctl + +`stackctl` is built so your team can add its own subcommands without forking. Drop an executable named `stackctl-` on your `$PATH` and it becomes `stackctl ` automatically — same pattern as `git`, `kubectl`, and `gh`. + +Because the mechanism is "any executable with the right name", plugins can be written in **any language** (shell, Python, Go, Node, Rust, …) and distributed however you already ship binaries to your team (package manager, tarball, Docker, private registry, rsync). + +--- + +## The 5-minute tutorial — your first plugin + +### 1. Write the plugin + +```bash +#!/usr/bin/env bash +# stackctl-hello — minimal stackctl plugin +set -euo pipefail + +cat <} + API key: ${STACKCTL_API_KEY:+***configured***}${STACKCTL_API_KEY:-} + args: $* +MSG +``` + +Save it anywhere on your `PATH`: + +```bash +install -m 0755 stackctl-hello ~/.local/bin/ +``` + +### 2. Use it + +```bash +stackctl hello world +# → Hello from a stackctl plugin! +# API URL: http://localhost:8081 +# API key: ***configured*** +# args: world +``` + +```bash +stackctl --help | grep hello +# → hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) +``` + +That's the whole mechanism. Your binary gets exec'd with: + +- Whatever argv the user typed after `stackctl ` (plus flags stackctl didn't consume at the top level) +- `stdin`, `stdout`, `stderr` wired through directly +- The full parent environment (so `STACKCTL_API_URL` / `STACKCTL_API_KEY` are visible) +- The plugin's exit code becomes stackctl's exit code + +--- + +## What plugins are for + +Any team-specific or company-specific workflow that doesn't belong in core stackctl. Core stackctl knows how to speak the k8s-stack-manager API — nothing beyond that. If your workflow involves **your** infrastructure, your service catalog, your CMDB, your pager, your Slack — it belongs in a plugin. + +**Good plugin candidates:** + +- **Thin wrappers around actions.** `stackctl refresh-db ` POSTs to `/api/v1/stack-instances/:id/actions/refresh-db` and pretty-prints the result. Pairs with a webhook handler registered on the server side. +- **Operations that combine multiple stackctl calls** into a single higher-level command (e.g. "create + deploy + wait for healthy + run smoke test"). +- **Company-specific reporting.** `stackctl stacks-costing-money` that lists instances + pulls cost data from your FinOps API. +- **Developer conveniences.** `stackctl port-forward ` that resolves the stack namespace and wires up a `kubectl port-forward` for common services. +- **Integration with out-of-band systems.** Jira, Linear, Datadog, PagerDuty — anything stackctl shouldn't know about directly. + +**Bad plugin candidates:** + +- Things core stackctl already does (shadowing is prevented — built-in subcommands always win). +- Infrastructure changes. That's k8s-stack-manager's job. +- Logic that should live on the server side — e.g. orchestration that needs cluster credentials. Put that in an **action webhook** on the k8s-stack-manager side and have the plugin call it. See [k8s-stack-manager EXTENDING.md](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md). + +--- + +## Naming convention + +Plugin names follow a strict rule: + +- Binary on PATH: `stackctl-` +- Appears as: `stackctl ` +- `` can contain letters, digits, and dashes — e.g. `stackctl-refresh-db`, `stackctl-port-forward`, `stackctl-sync-cmdb` +- **Built-in commands always win.** If you create `stackctl-config`, it'll be shadowed by the built-in `stackctl config`. Pick a name that doesn't collide. + +Discovery is **first-PATH-wins** (standard PATH semantics). If `stackctl-hello` exists in two PATH entries, the earlier one is used; the later one is silently ignored. + +## What plugins receive + +### Environment variables (inherited) + +The plugin inherits stackctl's entire environment. In particular: + +| Variable | Purpose | +|---|---| +| `STACKCTL_API_URL` | Base URL of the k8s-stack-manager API | +| `STACKCTL_API_KEY` | API key (header `X-API-Key`) | +| `STACKCTL_INSECURE` | `1` to skip TLS verification | +| `HOME`, `PATH`, `LANG`, … | The rest of the user's shell environment | + +### Arguments + +Whatever the user typed after `stackctl `. `stackctl --help ` shows the plugin in the command list; `stackctl --help` is delegated to the plugin's own help. + +### Stdin/stdout/stderr + +Wired through directly. A plugin can prompt for confirmation, pipe into another command, or print JSON — whatever makes sense. + +### Exit code + +Propagated back. Non-zero exit from the plugin fails the outer `stackctl` invocation, matching every other Unix tool. + +--- + +## Recipe: invoking a custom action + +The common pattern. You have an action webhook registered on the k8s-stack-manager side (see [server-side extending](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md)); you want a stackctl plugin that invokes it. + +### Bash (no dependencies) + +```bash +#!/usr/bin/env bash +# stackctl-snapshot-pvc — invokes the snapshot-pvc action +set -euo pipefail + +INSTANCE_ID=${1:?usage: stackctl snapshot-pvc } + +: "${STACKCTL_API_URL:?STACKCTL_API_URL not set — run 'stackctl config set api-url ...' first}" + +# Optional: allow insecure TLS per env +CURL_OPTS=() +[ "${STACKCTL_INSECURE:-}" = "1" ] && CURL_OPTS+=(--insecure) + +curl -sS "${CURL_OPTS[@]}" \ + -X POST "${STACKCTL_API_URL%/}/api/v1/stack-instances/${INSTANCE_ID}/actions/snapshot-pvc" \ + -H "Content-Type: application/json" \ + -H "X-API-Key: ${STACKCTL_API_KEY:-}" \ + -d '{}' +echo +``` + +### Python (stdlib only) + +```python +#!/usr/bin/env python3 +# stackctl-snapshot-pvc — invokes the snapshot-pvc action +import json, os, ssl, sys +from urllib import request + +if len(sys.argv) < 2: + sys.exit("usage: stackctl snapshot-pvc ") +instance_id = sys.argv[1] + +api = os.environ.get("STACKCTL_API_URL", "").rstrip("/") +if not api: + sys.exit("STACKCTL_API_URL not set") + +req = request.Request( + f"{api}/api/v1/stack-instances/{instance_id}/actions/snapshot-pvc", + data=b"{}", + method="POST", + headers={ + "Content-Type": "application/json", + "X-API-Key": os.environ.get("STACKCTL_API_KEY", ""), + }, +) +ctx = None +if os.environ.get("STACKCTL_INSECURE") == "1": + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE + +with request.urlopen(req, timeout=30, context=ctx) as resp: + body = json.loads(resp.read()) + print(json.dumps(body, indent=2)) +``` + +### Go (compiled, single binary) + +```go +// stackctl-snapshot-pvc/main.go — compile: go build -o stackctl-snapshot-pvc +package main + +import ( + "bytes" + "crypto/tls" + "fmt" + "io" + "net/http" + "os" +) + +func main() { + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "usage: stackctl snapshot-pvc ") + os.Exit(2) + } + api := os.Getenv("STACKCTL_API_URL") + if api == "" { + fmt.Fprintln(os.Stderr, "STACKCTL_API_URL not set") + os.Exit(1) + } + url := fmt.Sprintf("%s/api/v1/stack-instances/%s/actions/snapshot-pvc", api, os.Args[1]) + req, _ := http.NewRequest("POST", url, bytes.NewReader([]byte("{}"))) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Key", os.Getenv("STACKCTL_API_KEY")) + c := &http.Client{} + if os.Getenv("STACKCTL_INSECURE") == "1" { + c.Transport = &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}} + } + resp, err := c.Do(req) + if err != nil { + fmt.Fprintln(os.Stderr, "request:", err) + os.Exit(1) + } + defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + fmt.Print(string(body)) + if resp.StatusCode >= 400 { + os.Exit(1) + } +} +``` + +--- + +## Recipe: combining multiple stackctl calls + +For workflow plugins that orchestrate the built-in commands, just exec `stackctl` back into itself: + +```bash +#!/usr/bin/env bash +# stackctl-deploy-and-wait — create + deploy + wait until Running +set -euo pipefail + +DEF_ID=${1:?usage: stackctl deploy-and-wait } +NAME=${2:?usage: stackctl deploy-and-wait } + +# Create +INST_ID=$(stackctl stack create --definition "$DEF_ID" --name "$NAME" --quiet) +echo "Created $INST_ID" + +# Deploy +stackctl stack deploy "$INST_ID" --quiet + +# Poll until Running (or Error) +while :; do + STATUS=$(stackctl stack get "$INST_ID" -o json | jq -r .status) + case "$STATUS" in + running) echo "$INST_ID is running."; break ;; + error) echo "deploy failed: $(stackctl stack get "$INST_ID" -o json | jq -r .error_message)"; exit 1 ;; + *) echo "status=$STATUS, waiting..."; sleep 5 ;; + esac +done +``` + +`stackctl stack get -o json` makes this composable — the built-in commands are designed for scripting. + +--- + +## Best practices + +### Respect `--quiet` + +If the user sets `STACKCTL_QUIET=1` (or runs your plugin with `--quiet`), minimise output — print just the IDs or the raw result. Match core stackctl's behaviour. + +### Honour `-o json` + +When it makes sense, accept `-o json` and emit structured output. Downstream scripts can then `jq` into your plugin's result the same way they do with built-ins. + +### Don't re-implement auth + +Read `STACKCTL_API_URL` + `STACKCTL_API_KEY` directly. If the user has a stackctl context configured, those env vars pass through. If they haven't, fail fast with a helpful message (`Configure via stackctl config set api-url `) — don't try to parse `~/.stackmanager/config.yaml` yourself. + +### Use a deny list for dangerous defaults + +If your plugin mutates state (deletes, force-redeploys, wipes data), make it interactive or require `--yes`. The core stackctl conventions are: + +- `-y` / `--yes` skips the "are you sure?" prompt +- No-flag means prompt; prompt reads from stdin +- A full "destructive" operation also prints a summary of what will happen before the prompt + +### Emit JSON responses verbatim on `-o json` + +When the server returns a response body, forward it unmodified rather than reshaping it. Lets users write stable jq expressions that survive plugin version bumps. + +### Install to a consistent location + +Document where your plugin should live. Common choices: + +- `~/.local/bin/` (user-scoped; already on most PATHs via `~/.profile`) +- `/usr/local/bin/` (system-wide; requires sudo) +- A project-managed `bin/` dir added to PATH in your team's shell profile + +### Version your plugin independently + +Plugins and stackctl evolve separately. Tag releases, publish changelogs, keep a `--version` flag so users can report what they're running when they file bugs. + +--- + +## Distribution + +Because plugins are just executables, you ship them however your team already ships CLIs. + +### Simple: a tarball or Docker image + +```bash +# Install script +curl -sSfL https://your-host/install-plugin.sh | bash +``` + +The install script drops the binary in `~/.local/bin/` and that's it. + +### Homebrew / apt / yum + +Normal package manager flow. No plugin-specific infrastructure needed. + +### Kubernetes / container-based distribution + +If your org already distributes internal tools as container images, bake the plugin into an image the user pulls and copies out: + +```bash +docker cp $(docker create your-company/stackctl-plugins:latest):/bin/stackctl-refresh-db ~/.local/bin/ +``` + +### Team-internal: checked into a dotfiles repo + +For small teams, plugin source lives in a shared dotfiles repo and ships via each developer's bootstrap script. + +--- + +## How it works internally + +On `Execute()`, stackctl scans every directory in `$PATH`. For each regular executable file whose name starts with `stackctl-`: + +1. Strip the `stackctl-` prefix to get the plugin name. +2. Skip if a built-in subcommand with the same name is already registered. +3. Register a new Cobra subcommand: + - `Use: ` + - `Short: "Plugin: ()"` + - `DisableFlagParsing: true` (plugin handles its own flags) + - `RunE`: exec the binary with the remaining args, piping I/O, propagating exit code + +If the user runs `stackctl …`, Cobra routes to the registered subcommand's `RunE`, which exec's the plugin. + +Source: [cli/cmd/plugins.go](cli/cmd/plugins.go) (≈110 lines). No plugin framework, no SDK — just `os/exec` + `$PATH`. + +--- + +## Troubleshooting + +### "unknown command" but my plugin exists + +- Check the binary is **executable** (`chmod +x`). Non-executable files in PATH are ignored. +- Check the directory is actually in `$PATH`. `which stackctl-` should find it. +- Check the name starts with `stackctl-` (not `stackctl_` or `stackctl `). +- Shadow check: is there a built-in `stackctl `? `stackctl --help | grep ''` — if the Short text doesn't say "Plugin:", a built-in is winning. + +### Plugin runs but `$STACKCTL_API_URL` is empty + +The user hasn't configured it. Either: +- They should `stackctl config set api-url ` and re-export the env var +- Your plugin should fall back to reading `~/.stackmanager/config.yaml` and then the active context's `api-url` + +Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless you parse the config yourself. + +### Plugin exits non-zero but stackctl exits 0 + +The plugin ran but didn't fail. Check the plugin's own error handling. `bash -x` or `set -x` inside a shell plugin is a quick way to see what happened. + +### Plugin shadows a built-in + +Built-ins always win. Rename the plugin to something that doesn't collide. (This is a safety feature — otherwise a malicious `stackctl-config` on PATH could intercept credentials.) + +--- + +## Related + +- [k8s-stack-manager EXTENDING.md](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md) — the server side: how to register a webhook handler that a stackctl plugin can invoke. Most plugins pair with an action webhook; this is the complete picture. +- [cli/cmd/plugins.go](cli/cmd/plugins.go) — plugin-discovery implementation. +- [cli/cmd/plugins_test.go](cli/cmd/plugins_test.go) — test patterns for plugin behaviour. diff --git a/README.md b/README.md index 5413c32..69e0330 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,30 @@ stackctl template quick-deploy 1 stackctl stack list --mine ``` +## Extending — add your own subcommands + +Drop an executable named `stackctl-` anywhere on your `$PATH` and it becomes `stackctl ` automatically. No plugin SDK, no recompile, no changes to stackctl itself. Same pattern as `git`, `kubectl`, and `gh`. + +Because the contract is "any executable with the right name", plugins can be written in any language (shell, Python, Go, Node, Rust, …) and shipped however your team already distributes binaries. + +Quick example: + +```bash +cat > ~/.local/bin/stackctl-hello <<'EOF' +#!/usr/bin/env bash +echo "Hello! API=${STACKCTL_API_URL} args=$*" +EOF +chmod +x ~/.local/bin/stackctl-hello + +stackctl hello world # → Hello! API=http://... args=world +stackctl --help | grep hello +# hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) +``` + +The plugin inherits `STACKCTL_API_URL`, `STACKCTL_API_KEY`, and the rest of the user's environment. Built-in subcommands always win on name collisions (a safety feature — a malicious `stackctl-config` on PATH can't intercept credentials). + +👉 **[Full guide: EXTENDING.md](EXTENDING.md)** — tutorial, recipes in bash/Python/Go, best practices, and how plugins pair with [server-side action webhooks](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md) for end-to-end custom operations. + ## Configuration stackctl uses named contexts to manage multiple environments. Configuration is stored in `~/.stackmanager/config.yaml`. From 8f1030cb775e38efbf34bdc5fd30f9ecc3549d62 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 10:28:00 +0200 Subject: [PATCH 05/10] chore(plugin): address code-review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Forward --insecure, --quiet, --output into plugin env as STACKCTL_INSECURE, STACKCTL_QUIET, STACKCTL_OUTPUT so plugins see the same effective config stackctl uses for built-in commands. EXTENDING.md advertised this contract but the code didn't deliver it. - Hide the absolute plugin path from --help (was leaking $HOME in screenshots). Full path still visible in `stackctl help ` Long text for debugging. - Reject nil FormatterFunc / SingleFormatterFunc at RegisterFormat time so misuse surfaces at init, not inside a render path minutes later. - Strengthen plugin collision test: assert exactly one command named "config" exists (not two with Cobra silently accepting the dupe). - Add stdin-passthrough test using a cat-style plugin — the production code already routes stdin via cmd.InOrStdin() but that contract was previously untested. - Polish Python recipe in EXTENDING.md: proper HTTPError/URLError handling + stderr warning when STACKCTL_INSECURE is active. - Expand the "Environment variables inherited" section with the full resolved-from-flags table and a security note that the entire parent environment (including unrelated creds) is forwarded to every plugin. All 6 test packages green. --- EXTENDING.md | 60 ++++++++++++++++++++++---------- cli/cmd/plugins.go | 39 +++++++++++++++++++-- cli/cmd/plugins_test.go | 39 ++++++++++++++++++++- cli/pkg/output/formatter_test.go | 15 +++++++- cli/pkg/output/output.go | 12 +++++-- 5 files changed, 140 insertions(+), 25 deletions(-) diff --git a/EXTENDING.md b/EXTENDING.md index f6094cc..62cf61b 100644 --- a/EXTENDING.md +++ b/EXTENDING.md @@ -86,16 +86,27 @@ Discovery is **first-PATH-wins** (standard PATH semantics). If `stackctl-hello` ## What plugins receive -### Environment variables (inherited) - -The plugin inherits stackctl's entire environment. In particular: - -| Variable | Purpose | -|---|---| -| `STACKCTL_API_URL` | Base URL of the k8s-stack-manager API | -| `STACKCTL_API_KEY` | API key (header `X-API-Key`) | -| `STACKCTL_INSECURE` | `1` to skip TLS verification | -| `HOME`, `PATH`, `LANG`, … | The rest of the user's shell environment | +### Environment variables (inherited + resolved) + +The plugin inherits **stackctl's entire environment** (same pattern as `git`, +`kubectl`, `gh`). On top of that, stackctl resolves its persistent flags into +env vars before exec so a plugin sees the same effective configuration it +would if stackctl invoked a built-in command: + +| Variable | Source | Purpose | +|---|---|---| +| `STACKCTL_API_URL` | user env / stackctl config | Base URL of the k8s-stack-manager API | +| `STACKCTL_API_KEY` | user env / stackctl config | API key (header `X-API-Key`) | +| `STACKCTL_INSECURE` | `--insecure` flag OR user env | `1` to skip TLS verification | +| `STACKCTL_QUIET` | `--quiet` flag | `1` when the user requested quiet output | +| `STACKCTL_OUTPUT` | `--output` flag | `table` / `json` / `yaml` / a registered custom format | +| `HOME`, `PATH`, `LANG`, `AWS_*`, `KUBECONFIG`, … | parent shell | the rest of the user's environment | + +> **Security note:** because the full parent environment is forwarded, +> plugins have access to credentials stackctl doesn't know about +> (AWS_ACCESS_KEY_ID, GITHUB_TOKEN, KUBECONFIG contents, …). Install +> plugins from sources you trust. This matches the `git`/`kubectl` +> security model. ### Arguments @@ -145,6 +156,7 @@ echo # stackctl-snapshot-pvc — invokes the snapshot-pvc action import json, os, ssl, sys from urllib import request +from urllib.error import HTTPError, URLError if len(sys.argv) < 2: sys.exit("usage: stackctl snapshot-pvc ") @@ -154,6 +166,14 @@ api = os.environ.get("STACKCTL_API_URL", "").rstrip("/") if not api: sys.exit("STACKCTL_API_URL not set") +if os.environ.get("STACKCTL_INSECURE") == "1": + print("WARNING: TLS verification disabled (STACKCTL_INSECURE=1)", file=sys.stderr) + ctx = ssl.create_default_context() + ctx.check_hostname = False + ctx.verify_mode = ssl.CERT_NONE +else: + ctx = None + req = request.Request( f"{api}/api/v1/stack-instances/{instance_id}/actions/snapshot-pvc", data=b"{}", @@ -163,15 +183,19 @@ req = request.Request( "X-API-Key": os.environ.get("STACKCTL_API_KEY", ""), }, ) -ctx = None -if os.environ.get("STACKCTL_INSECURE") == "1": - ctx = ssl.create_default_context() - ctx.check_hostname = False - ctx.verify_mode = ssl.CERT_NONE -with request.urlopen(req, timeout=30, context=ctx) as resp: - body = json.loads(resp.read()) - print(json.dumps(body, indent=2)) +try: + with request.urlopen(req, timeout=30, context=ctx) as resp: + body = json.loads(resp.read()) +except HTTPError as e: # 4xx/5xx — read body for details + body = {"error": e.reason, "status": e.code, "body": e.read().decode(errors="replace")} + print(json.dumps(body, indent=2)); sys.exit(1) +except URLError as e: + sys.exit(f"request failed: {e.reason}") + +# Echo server response. If the user set STACKCTL_OUTPUT=json we keep the +# raw shape; for other values we could reshape into a friendlier table. +print(json.dumps(body, indent=2)) ``` ### Go (compiled, single binary) diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go index 0f5c996..7900410 100644 --- a/cli/cmd/plugins.go +++ b/cli/cmd/plugins.go @@ -74,6 +74,32 @@ func discoverPlugins(pathEnv string) map[string]string { return found } +// pluginEnv returns the environment to pass to a plugin subprocess. It +// preserves the full parent environment — plugins might legitimately need +// unrelated variables (AWS_PROFILE, KUBECONFIG, etc.) — and injects +// STACKCTL_* values resolved from flags so a plugin sees the same effective +// config stackctl itself would use for a built-in command. +// +// Flag-to-env wiring documented in EXTENDING.md as a plugin-author contract. +func pluginEnv(cmd *cobra.Command) []string { + env := os.Environ() + // Resolve --insecure (flag > env > config). The config layer may not be + // loaded yet for plugin commands because persistent pre-run depends on + // the command name, so favor the flag value here. + if cmd != nil { + if insecure, err := cmd.Root().PersistentFlags().GetBool("insecure"); err == nil && insecure { + env = append(env, "STACKCTL_INSECURE=1") + } + if quiet, err := cmd.Root().PersistentFlags().GetBool("quiet"); err == nil && quiet { + env = append(env, "STACKCTL_QUIET=1") + } + if output, err := cmd.Root().PersistentFlags().GetString("output"); err == nil && output != "" { + env = append(env, "STACKCTL_OUTPUT="+output) + } + } + return env +} + // existingCommandNames returns the set of top-level subcommand names already // registered on root, so discovery can avoid clobbering built-ins. func existingCommandNames(root *cobra.Command) map[string]struct{} { @@ -90,19 +116,26 @@ func existingCommandNames(root *cobra.Command) map[string]struct{} { // newPluginCommand wraps an external binary as a Cobra subcommand. The binary // receives all arguments after the plugin name; stdin/stdout/stderr pass // through directly, and the exit code propagates to the caller. +// +// binaryPath is captured at discovery time so later PATH modifications can't +// rebind which binary we exec — the absolute path we resolved in +// discoverPlugins is the exact path we run. func newPluginCommand(name, binaryPath string) *cobra.Command { return &cobra.Command{ + // Hide the absolute path from --help listings (leaks home dir in screenshots). + // The full path is in Long so `stackctl help ` still reveals it for debugging. Use: name, - Short: "Plugin: " + name + " (" + binaryPath + ")", + Short: "Plugin: " + name, + Long: "External plugin resolved to " + binaryPath, DisableFlagParsing: true, SilenceUsage: true, SilenceErrors: true, RunE: func(cmd *cobra.Command, args []string) error { - proc := exec.Command(binaryPath, args...) //nolint:gosec // binary path discovered from PATH, not user input + proc := exec.Command(binaryPath, args...) //nolint:gosec // absolute path captured at discovery time; rebinding via PATH is impossible after that point proc.Stdin = cmd.InOrStdin() proc.Stdout = cmd.OutOrStdout() proc.Stderr = cmd.ErrOrStderr() - proc.Env = os.Environ() + proc.Env = pluginEnv(cmd) if err := proc.Run(); err != nil { if exitErr, ok := err.(*exec.ExitError); ok { // Propagate the plugin's exit code. Cobra will surface diff --git a/cli/cmd/plugins_test.go b/cli/cmd/plugins_test.go index f6beffc..8d1558f 100644 --- a/cli/cmd/plugins_test.go +++ b/cli/cmd/plugins_test.go @@ -6,6 +6,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strings" "testing" "github.com/spf13/cobra" @@ -111,13 +112,17 @@ func TestRegisterPlugins_BuiltinWinsOnCollision(t *testing.T) { registerPlugins(root, dir) + // Verify exactly one command named "config" exists (not two, with Cobra + // silently accepting the duplicate) and that it's the built-in. + named := 0 var found *cobra.Command for _, c := range root.Commands() { if c.Name() == "config" { + named++ found = c - break } } + assert.Equal(t, 1, named, "exactly one command should be named 'config'") require.NotNil(t, found) assert.Equal(t, "builtin", found.Short, "built-in must not be replaced by a colliding plugin") } @@ -159,6 +164,38 @@ func TestRegisterPlugins_NoOpOnEmptyPath(t *testing.T) { assert.Equal(t, before, len(root.Commands())) } +// TestRegisterPlugins_StdinPassthrough proves stdin is routed to the plugin. +// Uses a `cat` style shell script that copies stdin to stdout; the plugin +// subcommand reads stdin via cmd.InOrStdin() which Cobra resolves to the +// buffer we set via root.SetIn. +func TestRegisterPlugins_StdinPassthrough(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + _ = writeScript(t, dir, "stackctl-cat", "#!/bin/sh\ncat\n") + + root := &cobra.Command{Use: "stackctl", SilenceUsage: true, SilenceErrors: true} + registerPlugins(root, dir) + + var catCmd *cobra.Command + for _, c := range root.Commands() { + if c.Name() == "cat" { + catCmd = c + break + } + } + require.NotNil(t, catCmd) + + var outBuf bytes.Buffer + root.SetIn(strings.NewReader("payload-from-stdin")) + root.SetOut(&outBuf) + root.SetErr(&outBuf) + + require.NoError(t, catCmd.RunE(catCmd, nil)) + assert.Equal(t, "payload-from-stdin", outBuf.String(), + "plugin must receive stdin and its stdout must reach root's writer") +} + // TestRegisterPlugins_RunViaCobraRouting wires a plugin through Cobra's // Execute path to verify the command is actually routed by name. The plugin // exits 0, so the parent survives; args are captured via a sentinel file diff --git a/cli/pkg/output/formatter_test.go b/cli/pkg/output/formatter_test.go index 63c5556..2303d44 100644 --- a/cli/pkg/output/formatter_test.go +++ b/cli/pkg/output/formatter_test.go @@ -28,17 +28,30 @@ func unregisterForTest(t *testing.T, names ...string) { func TestRegisterFormat_PanicOnBuiltinOverride(t *testing.T) { t.Parallel() + // Pass a non-nil fn so the built-in-name check fires first — otherwise + // the nil-fn guard would shadow the error we're asserting. + noop := func(io.Writer, interface{}, []string, [][]string) error { return nil } for _, name := range []string{"table", "json", "yaml"} { name := name t.Run(name, func(t *testing.T) { t.Parallel() assert.PanicsWithValue(t, fmt.Sprintf("output: cannot override built-in format %q", name), - func() { RegisterFormat(name, nil) }) + func() { RegisterFormat(name, noop) }) }) } } +func TestRegisterFormat_PanicOnNilFunc(t *testing.T) { + t.Parallel() + assert.PanicsWithValue(t, + `output: nil FormatterFunc for format "customnil"`, + func() { RegisterFormat("customnil", nil) }) + assert.PanicsWithValue(t, + `output: nil SingleFormatterFunc for format "customnil"`, + func() { RegisterSingleFormat("customnil", nil) }) +} + func TestNewPrinter_UsesCustomFormat(t *testing.T) { t.Parallel() RegisterFormat("csv", func(w io.Writer, _ interface{}, headers []string, rows [][]string) error { diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index bbbacb9..6efbd92 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -42,11 +42,16 @@ var ( // render so adds during rendering are racy. // // Name must not collide with the built-in formats (table, json, yaml). -// Passing a built-in name panics to surface the mistake early. +// Passing a built-in name panics to surface the mistake early. A nil fn +// also panics at registration time, so the "which handler?" error surfaces +// at startup rather than inside a render call that could be minutes later. func RegisterFormat(name string, fn FormatterFunc) { if name == string(FormatTable) || name == string(FormatJSON) || name == string(FormatYAML) { panic(fmt.Sprintf("output: cannot override built-in format %q", name)) } + if fn == nil { + panic(fmt.Sprintf("output: nil FormatterFunc for format %q", name)) + } formatRegistryMu.Lock() defer formatRegistryMu.Unlock() formatRegistry[name] = fn @@ -54,8 +59,11 @@ func RegisterFormat(name string, fn FormatterFunc) { // RegisterSingleFormat attaches a custom single-item formatter for an // already-registered format. Optional — only needed when the list and -// single shapes genuinely differ. +// single shapes genuinely differ. A nil fn panics at registration time. func RegisterSingleFormat(name string, fn SingleFormatterFunc) { + if fn == nil { + panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", name)) + } formatRegistryMu.Lock() defer formatRegistryMu.Unlock() singleRegistry[name] = fn From c12b62b33e6e8dfd480992084b275f25dbcb1c4a Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 11:00:59 +0200 Subject: [PATCH 06/10] chore(plugin): address Copilot review feedback on PR #35 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plugin code fixes: - discoverPlugins now resolves binaryPath to an absolute path via filepath.Abs, so relative $PATH entries can't leave a relative path stored as the plugin's captured exec target (Copilot #1). - Validate plugin name against [a-z0-9][a-z0-9-]* to reject filenames with whitespace or leading dashes (stackctl--help etc.) that would break Cobra routing (Copilot #12). - Windows portability: skip the POSIX executable-bit check on GOOS windows and trim .exe from the plugin name so stackctl-foo.exe becomes `stackctl foo` the same way it does on Unix (Copilot #7). Output registry fixes: - Normalise format names via strings.ToLower + TrimSpace in RegisterFormat / RegisterSingleFormat / lookups and the built-in collision check. Previously RegisterFormat("CSV", ...) was unreachable because NewPrinter lowercases `-o csv`; now case and leading/trailing whitespace don't matter (Copilot #5). Added 2 tests covering the new contract: `RegisterFormat(" CSV ", fn)` is reached by `-o csv`, and `RegisterFormat("JSON", ...)` panics the same way as "json". - Reword the "racy" comment — the registry IS synchronised, just not safe semantically (inconsistent renders across concurrent calls). Previous wording implied a data race (Copilot #4). Doc fixes (stale help output + API_URL semantics): - README.md and EXTENDING.md: example help output no longer shows the plugin's absolute path — matches current Short string (Copilot #2, #8). - EXTENDING.md "internals" section now documents that Short is short and Long carries the resolved path (Copilot #10). - EXTENDING.md env-vars section clarifies that STACKCTL_API_URL / STACKCTL_API_KEY are inherited from the parent shell ONLY — stackctl does not resolve them from ~/.stackmanager/config.yaml when execing plugins (Copilot #9, #11). Troubleshooting section updated with two recommended workflows: explicit export, or plugin-side config parse. - EXTENDING.md arguments section fixes `--help ` (not a real Cobra invocation) to `help ` (Copilot #3). Test convention fix: - cli/cmd/plugins_test.go: remove all t.Parallel() calls per project convention — cmd/ tests mutate package-level globals and must run serially. The .github/instructions/tests.instructions.md file explicitly documents this (Copilot #6). All 6 stackctl test packages pass. go vet clean. --- EXTENDING.md | 55 ++++++++++++++++++++++++-------- README.md | 2 +- cli/cmd/plugins.go | 34 +++++++++++++++++--- cli/cmd/plugins_test.go | 10 ------ cli/pkg/output/formatter_test.go | 37 +++++++++++++++++++++ cli/pkg/output/output.go | 36 ++++++++++++++------- 6 files changed, 134 insertions(+), 40 deletions(-) diff --git a/EXTENDING.md b/EXTENDING.md index 62cf61b..b3fc5cf 100644 --- a/EXTENDING.md +++ b/EXTENDING.md @@ -41,9 +41,11 @@ stackctl hello world ```bash stackctl --help | grep hello -# → hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) +# → hello Plugin: hello ``` +The plugin's absolute path is kept out of `--help` (it was leaking `$HOME` in screenshots). Use `stackctl help hello` to see the full resolved path. + That's the whole mechanism. Your binary gets exec'd with: - Whatever argv the user typed after `stackctl ` (plus flags stackctl didn't consume at the top level) @@ -86,22 +88,28 @@ Discovery is **first-PATH-wins** (standard PATH semantics). If `stackctl-hello` ## What plugins receive -### Environment variables (inherited + resolved) +### Environment variables (inherited + flag-derived) The plugin inherits **stackctl's entire environment** (same pattern as `git`, -`kubectl`, `gh`). On top of that, stackctl resolves its persistent flags into -env vars before exec so a plugin sees the same effective configuration it -would if stackctl invoked a built-in command: +`kubectl`, `gh`). On top of that, stackctl exports a small set of values +**derived from flags** (not from `~/.stackmanager/config.yaml`) before exec, +so plugins can observe the user's requested TLS / output behaviour: | Variable | Source | Purpose | |---|---|---| -| `STACKCTL_API_URL` | user env / stackctl config | Base URL of the k8s-stack-manager API | -| `STACKCTL_API_KEY` | user env / stackctl config | API key (header `X-API-Key`) | -| `STACKCTL_INSECURE` | `--insecure` flag OR user env | `1` to skip TLS verification | +| `STACKCTL_API_URL` | parent shell only | Base URL of the k8s-stack-manager API, **if already exported** by the user | +| `STACKCTL_API_KEY` | parent shell only | API key (header `X-API-Key`), **if already exported** by the user | +| `STACKCTL_INSECURE` | `--insecure` flag OR parent shell | `1` to skip TLS verification | | `STACKCTL_QUIET` | `--quiet` flag | `1` when the user requested quiet output | | `STACKCTL_OUTPUT` | `--output` flag | `table` / `json` / `yaml` / a registered custom format | | `HOME`, `PATH`, `LANG`, `AWS_*`, `KUBECONFIG`, … | parent shell | the rest of the user's environment | +> **stackctl does NOT inject values resolved from the stackctl config file.** +> If your plugin needs `STACKCTL_API_URL` / `STACKCTL_API_KEY`, they must +> already be present in the environment when stackctl is launched — they +> will not be filled in from `~/.stackmanager/config.yaml` automatically. +> See the Troubleshooting section below for recommended workflows. + > **Security note:** because the full parent environment is forwarded, > plugins have access to credentials stackctl doesn't know about > (AWS_ACCESS_KEY_ID, GITHUB_TOKEN, KUBECONFIG contents, …). Install @@ -110,7 +118,7 @@ would if stackctl invoked a built-in command: ### Arguments -Whatever the user typed after `stackctl `. `stackctl --help ` shows the plugin in the command list; `stackctl --help` is delegated to the plugin's own help. +Whatever the user typed after `stackctl `. Use `stackctl help ` for stackctl's built-in help entry (which shows the plugin's resolved absolute path); `stackctl --help` is delegated to the plugin's own help handling. ### Stdin/stdout/stderr @@ -364,6 +372,14 @@ On `Execute()`, stackctl scans every directory in `$PATH`. For each regular exec - `DisableFlagParsing: true` (plugin handles its own flags) - `RunE`: exec the binary with the remaining args, piping I/O, propagating exit code +Cobra subcommand registration is: + +- `Use: ` +- `Short: "Plugin: "` (path kept out of the summary listing) +- `Long`: includes the plugin's absolute path for `stackctl help ` +- `DisableFlagParsing: true` — plugin handles its own flags +- `RunE`: exec the binary with the remaining args, piping I/O, propagating exit code + If the user runs `stackctl …`, Cobra routes to the registered subcommand's `RunE`, which exec's the plugin. Source: [cli/cmd/plugins.go](cli/cmd/plugins.go) (≈110 lines). No plugin framework, no SDK — just `os/exec` + `$PATH`. @@ -381,11 +397,24 @@ Source: [cli/cmd/plugins.go](cli/cmd/plugins.go) (≈110 lines). No plugin frame ### Plugin runs but `$STACKCTL_API_URL` is empty -The user hasn't configured it. Either: -- They should `stackctl config set api-url ` and re-export the env var -- Your plugin should fall back to reading `~/.stackmanager/config.yaml` and then the active context's `api-url` +The plugin process only sees environment variables that were **already exported** in the user's shell. `stackctl config set api-url ` writes to `~/.stackmanager/config.yaml`; it does **not** export `STACKCTL_API_URL` into the environment for subsequent plugin execs. + +Pick one workflow and document it for your plugin's users: + +- **Have the user export env vars explicitly:** + + ```bash + export STACKCTL_API_URL="$(stackctl config get api-url)" + export STACKCTL_API_KEY="$(stackctl config get api-key)" + stackctl my-plugin … + ``` + +- **Have the plugin fall back to reading the config file itself:** + parse `~/.stackmanager/config.yaml` and resolve the active context's + `api-url` + `api-key` if the env vars are empty. A ~30-line helper in + any language. -Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless you parse the config yourself. +Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless you parse the config yourself. stackctl does **not** inject resolved config values when execing plugins. ### Plugin exits non-zero but stackctl exits 0 diff --git a/README.md b/README.md index 69e0330..c6d0709 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ chmod +x ~/.local/bin/stackctl-hello stackctl hello world # → Hello! API=http://... args=world stackctl --help | grep hello -# hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) +# hello Plugin: hello ``` The plugin inherits `STACKCTL_API_URL`, `STACKCTL_API_KEY`, and the rest of the user's environment. Built-in subcommands always win on name collisions (a safety feature — a malicious `stackctl-config` on PATH can't intercept credentials). diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go index 7900410..2fa48eb 100644 --- a/cli/cmd/plugins.go +++ b/cli/cmd/plugins.go @@ -5,6 +5,8 @@ import ( "os" "os/exec" "path/filepath" + "regexp" + "runtime" "strings" "github.com/spf13/cobra" @@ -14,6 +16,12 @@ import ( // plugin. A binary at PATH/stackctl-foo becomes the subcommand `stackctl foo`. const pluginPrefix = "stackctl-" +// pluginNamePattern restricts plugin names to ASCII letters, digits, and +// dashes so they form valid Cobra command names. A name like +// "stackctl- bad" (with whitespace) or "stackctl--help" breaks help +// routing and is skipped at discovery time. +var pluginNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) + // registerPlugins scans $PATH for executables named stackctl- and adds // each as a top-level subcommand that proxies to the external binary. // @@ -38,9 +46,18 @@ func registerPlugins(root *cobra.Command, pathEnv string) { } } -// discoverPlugins walks pathEnv (a colon-separated PATH) and returns a map -// of plugin name to absolute path. First-win semantics: earlier PATH entries +// discoverPlugins walks pathEnv (the process $PATH) and returns a map of +// plugin name to absolute path. First-win semantics: earlier PATH entries // take precedence over later ones when multiple binaries share a name. +// +// Paths are resolved to absolute via filepath.Abs so execution is safe +// regardless of relative entries in $PATH — the captured path is what we +// later exec, which means rebinding $PATH after this function runs cannot +// change which binary a plugin routes to. +// +// Windows: .exe suffix handling via PATHEXT is honoured; the POSIX +// executable-bit check is skipped because Windows represents executability +// via extension, not permission bits. func discoverPlugins(pathEnv string) map[string]string { found := make(map[string]string) for _, dir := range filepath.SplitList(pathEnv) { @@ -57,15 +74,24 @@ func discoverPlugins(pathEnv string) map[string]string { continue } pluginName := strings.TrimPrefix(name, pluginPrefix) + if runtime.GOOS == "windows" { + pluginName = strings.TrimSuffix(pluginName, ".exe") + } + if !pluginNamePattern.MatchString(pluginName) { + continue + } if _, seen := found[pluginName]; seen { continue } - full := filepath.Join(dir, name) + full, err := filepath.Abs(filepath.Join(dir, name)) + if err != nil { + continue + } info, err := os.Stat(full) if err != nil || !info.Mode().IsRegular() { continue } - if info.Mode().Perm()&0o111 == 0 { + if runtime.GOOS != "windows" && info.Mode().Perm()&0o111 == 0 { continue } found[pluginName] = full diff --git a/cli/cmd/plugins_test.go b/cli/cmd/plugins_test.go index 8d1558f..6c60c57 100644 --- a/cli/cmd/plugins_test.go +++ b/cli/cmd/plugins_test.go @@ -28,7 +28,6 @@ func writeScript(t *testing.T, dir, name, body string) string { } func TestDiscoverPlugins_FindsExecutableStackctlBinaries(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-hello", "#!/bin/sh\necho hi\n") @@ -44,7 +43,6 @@ func TestDiscoverPlugins_FindsExecutableStackctlBinaries(t *testing.T) { } func TestDiscoverPlugins_SkipsNonExecutable(t *testing.T) { - t.Parallel() dir := t.TempDir() nonExec := filepath.Join(dir, "stackctl-readonly") @@ -55,7 +53,6 @@ func TestDiscoverPlugins_SkipsNonExecutable(t *testing.T) { } func TestDiscoverPlugins_FirstPathEntryWins(t *testing.T) { - t.Parallel() dir1 := t.TempDir() dir2 := t.TempDir() @@ -68,7 +65,6 @@ func TestDiscoverPlugins_FirstPathEntryWins(t *testing.T) { } func TestDiscoverPlugins_IgnoresMissingAndEmptyPATHEntries(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-ok", "#!/bin/sh\necho ok\n") @@ -78,7 +74,6 @@ func TestDiscoverPlugins_IgnoresMissingAndEmptyPATHEntries(t *testing.T) { } func TestRegisterPlugins_AddsPluginAsSubcommand(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-greet", "#!/bin/sh\necho hello-from-plugin\n") @@ -101,7 +96,6 @@ func TestRegisterPlugins_AddsPluginAsSubcommand(t *testing.T) { } func TestRegisterPlugins_BuiltinWinsOnCollision(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-config", "#!/bin/sh\necho shadow\n") @@ -128,7 +122,6 @@ func TestRegisterPlugins_BuiltinWinsOnCollision(t *testing.T) { } func TestRegisterPlugins_PluginInvocationPassesThroughArgsAndStdout(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-echo", @@ -156,7 +149,6 @@ func TestRegisterPlugins_PluginInvocationPassesThroughArgsAndStdout(t *testing.T } func TestRegisterPlugins_NoOpOnEmptyPath(t *testing.T) { - t.Parallel() root := &cobra.Command{Use: "stackctl"} before := len(root.Commands()) @@ -169,7 +161,6 @@ func TestRegisterPlugins_NoOpOnEmptyPath(t *testing.T) { // subcommand reads stdin via cmd.InOrStdin() which Cobra resolves to the // buffer we set via root.SetIn. func TestRegisterPlugins_StdinPassthrough(t *testing.T) { - t.Parallel() dir := t.TempDir() _ = writeScript(t, dir, "stackctl-cat", "#!/bin/sh\ncat\n") @@ -201,7 +192,6 @@ func TestRegisterPlugins_StdinPassthrough(t *testing.T) { // exits 0, so the parent survives; args are captured via a sentinel file // the plugin writes. func TestRegisterPlugins_RunViaCobraRouting(t *testing.T) { - t.Parallel() dir := t.TempDir() sentinel := filepath.Join(t.TempDir(), "args.txt") diff --git a/cli/pkg/output/formatter_test.go b/cli/pkg/output/formatter_test.go index 2303d44..f029212 100644 --- a/cli/pkg/output/formatter_test.go +++ b/cli/pkg/output/formatter_test.go @@ -52,6 +52,43 @@ func TestRegisterFormat_PanicOnNilFunc(t *testing.T) { func() { RegisterSingleFormat("customnil", nil) }) } +// TestRegisterFormat_CaseInsensitiveAndTrimmed locks in the contract that +// RegisterFormat normalises the name the same way NewPrinter does when +// resolving --output. Without this, `RegisterFormat("CSV", ...)` would +// never match a user running `stackctl … -o csv`. +func TestRegisterFormat_CaseInsensitiveAndTrimmed(t *testing.T) { + t.Parallel() + called := 0 + fn := func(io.Writer, interface{}, []string, [][]string) error { + called++ + return nil + } + RegisterFormat(" CSV ", fn) + unregisterForTest(t, "csv") + + p := NewPrinter("csv", false, true) + p.Writer = io.Discard + assert.Equal(t, Format("csv"), p.Format, "NewPrinter should match case-insensitively") + require.NoError(t, p.Print(nil, nil, nil, nil)) + assert.Equal(t, 1, called, "formatter registered under 'CSV' should be reached via 'csv'") +} + +// TestRegisterFormat_BuiltinCollisionIsCaseInsensitive ensures the +// built-in check runs after normalisation, so `RegisterFormat("JSON", ...)` +// panics the same way as the lowercase form. +func TestRegisterFormat_BuiltinCollisionIsCaseInsensitive(t *testing.T) { + t.Parallel() + noop := func(io.Writer, interface{}, []string, [][]string) error { return nil } + for _, name := range []string{"JSON", " yaml", "Table "} { + name := name + t.Run(name, func(t *testing.T) { + t.Parallel() + assert.Panics(t, func() { RegisterFormat(name, noop) }, + "expected panic for built-in collision on %q", name) + }) + } +} + func TestNewPrinter_UsesCustomFormat(t *testing.T) { t.Parallel() RegisterFormat("csv", func(w io.Writer, _ interface{}, headers []string, rows [][]string) error { diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index 6efbd92..609d3c0 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -38,49 +38,61 @@ var ( ) // RegisterFormat attaches a named custom format. Call at init time, before -// any Printer is used concurrently — the registry is read-locked on every -// render so adds during rendering are racy. +// any Printer is used. The registry is synchronised so registration during +// rendering is not a Go data race, but doing so is unsupported and may lead +// to inconsistent behaviour across concurrent renders. // -// Name must not collide with the built-in formats (table, json, yaml). -// Passing a built-in name panics to surface the mistake early. A nil fn +// Name is normalised with strings.ToLower + TrimSpace before storage, so +// lookups match NewPrinter's case-insensitive resolution. Passing a built-in +// name (table, json, yaml) panics to surface the mistake early. A nil fn // also panics at registration time, so the "which handler?" error surfaces // at startup rather than inside a render call that could be minutes later. func RegisterFormat(name string, fn FormatterFunc) { - if name == string(FormatTable) || name == string(FormatJSON) || name == string(FormatYAML) { - panic(fmt.Sprintf("output: cannot override built-in format %q", name)) + norm := normalizeFormatName(name) + if norm == string(FormatTable) || norm == string(FormatJSON) || norm == string(FormatYAML) { + panic(fmt.Sprintf("output: cannot override built-in format %q", norm)) } if fn == nil { - panic(fmt.Sprintf("output: nil FormatterFunc for format %q", name)) + panic(fmt.Sprintf("output: nil FormatterFunc for format %q", norm)) } formatRegistryMu.Lock() defer formatRegistryMu.Unlock() - formatRegistry[name] = fn + formatRegistry[norm] = fn } // RegisterSingleFormat attaches a custom single-item formatter for an // already-registered format. Optional — only needed when the list and // single shapes genuinely differ. A nil fn panics at registration time. +// Names are normalised identically to RegisterFormat. func RegisterSingleFormat(name string, fn SingleFormatterFunc) { + norm := normalizeFormatName(name) if fn == nil { - panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", name)) + panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", norm)) } formatRegistryMu.Lock() defer formatRegistryMu.Unlock() - singleRegistry[name] = fn + singleRegistry[norm] = fn +} + +// normalizeFormatName is the single source of truth for turning a user- +// supplied format name into its registry key. Case-insensitive + whitespace +// tolerant. +func normalizeFormatName(name string) string { + return strings.ToLower(strings.TrimSpace(name)) } // lookupFormat returns the registered list formatter for name. func lookupFormat(name string) (FormatterFunc, bool) { formatRegistryMu.RLock() defer formatRegistryMu.RUnlock() - fn, ok := formatRegistry[name] + fn, ok := formatRegistry[normalizeFormatName(name)] return fn, ok } func lookupSingleFormat(name string) (SingleFormatterFunc, bool) { formatRegistryMu.RLock() defer formatRegistryMu.RUnlock() - fn, ok := singleRegistry[name] + fn, ok := singleRegistry[normalizeFormatName(name)] return fn, ok } From cdcb53d94b1ac55e727fa50cfd7ebccefe997361 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 15:17:46 +0200 Subject: [PATCH 07/10] fix(plugin): address Copilot review round 2 - EXTENDING.md: remove credential leak in tutorial bash snippet (the VAR-prefix-star pattern echoed the real API key when set) - EXTENDING.md: clarify that plugins do not inherit config-file auth, document env-var and stackctl-config-show strategies - EXTENDING.md: frame /actions/ as a k8s-stack-manager (external) endpoint, not a stackctl feature - output: NewPrinter uses the shared normalizeFormatName (TrimSpace+ToLower) so whitespace in the flag no longer mismatches the registry key - output: RegisterSingleFormat panics if base format not registered, catches silent wiring mistakes - plugins: flag-vs-env precedence honours an explicit --flag=false via PersistentFlags().Changed - plugins: clarify that only .exe is trimmed on Windows (not full PATHEXT) - plugins: align pluginNamePattern comment with the actual regex (lowercase) Co-Authored-By: Claude Opus 4.7 (1M context) --- EXTENDING.md | 20 ++++++++++-- cli/cmd/plugins.go | 66 +++++++++++++++++++++++++++++----------- cli/pkg/output/output.go | 10 ++++-- 3 files changed, 74 insertions(+), 22 deletions(-) diff --git a/EXTENDING.md b/EXTENDING.md index b3fc5cf..92d65ba 100644 --- a/EXTENDING.md +++ b/EXTENDING.md @@ -15,10 +15,17 @@ Because the mechanism is "any executable with the right name", plugins can be wr # stackctl-hello — minimal stackctl plugin set -euo pipefail +# Report a redacted marker for the API key; NEVER echo its value. +if [ -n "${STACKCTL_API_KEY:-}" ]; then + key_status='***configured***' +else + key_status='' +fi + cat <} - API key: ${STACKCTL_API_KEY:+***configured***}${STACKCTL_API_KEY:-} + API key: ${key_status} args: $* MSG ``` @@ -61,7 +68,7 @@ Any team-specific or company-specific workflow that doesn't belong in core stack **Good plugin candidates:** -- **Thin wrappers around actions.** `stackctl refresh-db ` POSTs to `/api/v1/stack-instances/:id/actions/refresh-db` and pretty-prints the result. Pairs with a webhook handler registered on the server side. +- **Thin wrappers around [k8s-stack-manager action webhooks](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md#action-webhooks).** The action endpoint (`POST /api/v1/stack-instances/:id/actions/:name`) is a k8s-stack-manager feature; stackctl itself has no action logic. A plugin like `stackctl refresh-db ` simply POSTs to that server endpoint and pretty-prints the response. Pair your plugin with the webhook handler you registered server-side. - **Operations that combine multiple stackctl calls** into a single higher-level command (e.g. "create + deploy + wait for healthy + run smoke test"). - **Company-specific reporting.** `stackctl stacks-costing-money` that lists instances + pulls cost data from your FinOps API. - **Developer conveniences.** `stackctl port-forward ` that resolves the stack namespace and wires up a `kubectl port-forward` for common services. @@ -301,7 +308,14 @@ When it makes sense, accept `-o json` and emit structured output. Downstream scr ### Don't re-implement auth -Read `STACKCTL_API_URL` + `STACKCTL_API_KEY` directly. If the user has a stackctl context configured, those env vars pass through. If they haven't, fail fast with a helpful message (`Configure via stackctl config set api-url `) — don't try to parse `~/.stackmanager/config.yaml` yourself. +Read `STACKCTL_API_URL` + `STACKCTL_API_KEY` from the environment. stackctl does **not** currently pass its config-file values into the plugin environment — only the flag-derived `STACKCTL_INSECURE`, `STACKCTL_QUIET`, and `STACKCTL_OUTPUT` flow through. So if the user configured their API URL via `stackctl config set api-url ` (writing `~/.stackmanager/config.yaml`) and never exported `STACKCTL_API_URL`, a plugin subprocess will not see it. + +Two workable strategies: + +1. **Require the env vars**, and fail fast with a pointer (`export STACKCTL_API_URL=... STACKCTL_API_KEY=...` — or wrap your plugin in a shell function that exports them). +2. **Shell out to `stackctl config show -o json`** and parse the result. Works without any env wiring, at the cost of one extra exec. + +Either way, don't parse `~/.stackmanager/config.yaml` directly — the schema is internal and may change. ### Use a deny list for dangerous defaults diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go index 2fa48eb..1e1f113 100644 --- a/cli/cmd/plugins.go +++ b/cli/cmd/plugins.go @@ -16,10 +16,12 @@ import ( // plugin. A binary at PATH/stackctl-foo becomes the subcommand `stackctl foo`. const pluginPrefix = "stackctl-" -// pluginNamePattern restricts plugin names to ASCII letters, digits, and -// dashes so they form valid Cobra command names. A name like -// "stackctl- bad" (with whitespace) or "stackctl--help" breaks help -// routing and is skipped at discovery time. +// pluginNamePattern restricts plugin names to lowercase ASCII letters, digits, +// and dashes, and requires the first character to be a letter or digit so names +// form valid Cobra command names. A name like "stackctl- bad" (with whitespace) +// or "stackctl--help" breaks help routing and is skipped at discovery time. +// Uppercase filenames (stackctl-Foo) are skipped — Cobra is case-sensitive and +// mixed-case subcommands are a footgun more than a feature. var pluginNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) // registerPlugins scans $PATH for executables named stackctl- and adds @@ -55,9 +57,10 @@ func registerPlugins(root *cobra.Command, pathEnv string) { // later exec, which means rebinding $PATH after this function runs cannot // change which binary a plugin routes to. // -// Windows: .exe suffix handling via PATHEXT is honoured; the POSIX -// executable-bit check is skipped because Windows represents executability -// via extension, not permission bits. +// Windows: discovery strips a trailing .exe so stackctl-foo.exe surfaces as +// the subcommand `foo`. Other PATHEXT extensions (.bat, .cmd, .ps1) are not +// currently recognised — if you need them, name the plugin binary .exe or +// front it with a .exe shim. func discoverPlugins(pathEnv string) map[string]string { found := make(map[string]string) for _, dir := range filepath.SplitList(pathEnv) { @@ -106,26 +109,55 @@ func discoverPlugins(pathEnv string) map[string]string { // STACKCTL_* values resolved from flags so a plugin sees the same effective // config stackctl itself would use for a built-in command. // +// Precedence: an explicitly-passed flag wins over a pre-existing env var. +// Flags that weren't set on the command line leave the inherited env +// untouched, so STACKCTL_INSECURE=1 in the parent shell keeps working for +// plugin invocations when no --insecure flag is passed. +// // Flag-to-env wiring documented in EXTENDING.md as a plugin-author contract. func pluginEnv(cmd *cobra.Command) []string { env := os.Environ() - // Resolve --insecure (flag > env > config). The config layer may not be - // loaded yet for plugin commands because persistent pre-run depends on - // the command name, so favor the flag value here. - if cmd != nil { - if insecure, err := cmd.Root().PersistentFlags().GetBool("insecure"); err == nil && insecure { - env = append(env, "STACKCTL_INSECURE=1") + if cmd == nil { + return env + } + flags := cmd.Root().PersistentFlags() + if flags.Changed("insecure") { + if insecure, err := flags.GetBool("insecure"); err == nil { + env = setEnv(env, "STACKCTL_INSECURE", boolEnvValue(insecure)) } - if quiet, err := cmd.Root().PersistentFlags().GetBool("quiet"); err == nil && quiet { - env = append(env, "STACKCTL_QUIET=1") + } + if flags.Changed("quiet") { + if quiet, err := flags.GetBool("quiet"); err == nil { + env = setEnv(env, "STACKCTL_QUIET", boolEnvValue(quiet)) } - if output, err := cmd.Root().PersistentFlags().GetString("output"); err == nil && output != "" { - env = append(env, "STACKCTL_OUTPUT="+output) + } + if flags.Changed("output") { + if output, err := flags.GetString("output"); err == nil { + env = setEnv(env, "STACKCTL_OUTPUT", output) } } return env } +// setEnv replaces (or appends) KEY=value in env, preserving order. +func setEnv(env []string, key, value string) []string { + prefix := key + "=" + for i, kv := range env { + if strings.HasPrefix(kv, prefix) { + env[i] = prefix + value + return env + } + } + return append(env, prefix+value) +} + +func boolEnvValue(b bool) string { + if b { + return "1" + } + return "0" +} + // existingCommandNames returns the set of top-level subcommand names already // registered on root, so discovery can avoid clobbering built-ins. func existingCommandNames(root *cobra.Command) map[string]struct{} { diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index 609d3c0..b2dfaea 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -62,7 +62,10 @@ func RegisterFormat(name string, fn FormatterFunc) { // RegisterSingleFormat attaches a custom single-item formatter for an // already-registered format. Optional — only needed when the list and -// single shapes genuinely differ. A nil fn panics at registration time. +// single shapes genuinely differ. A nil fn panics at registration time, +// as does registering a single formatter for a format that has no list +// formatter — that combination would silently fall back to a tabwriter +// and is almost always a wiring mistake. // Names are normalised identically to RegisterFormat. func RegisterSingleFormat(name string, fn SingleFormatterFunc) { norm := normalizeFormatName(name) @@ -71,6 +74,9 @@ func RegisterSingleFormat(name string, fn SingleFormatterFunc) { } formatRegistryMu.Lock() defer formatRegistryMu.Unlock() + if _, ok := formatRegistry[norm]; !ok { + panic(fmt.Sprintf("output: RegisterSingleFormat %q called before RegisterFormat — register the list formatter first", norm)) + } singleRegistry[norm] = fn } @@ -108,7 +114,7 @@ type Printer struct { // Falls back to FormatTable if the name is empty, unknown, or a misspelt // built-in. Custom formats registered via RegisterFormat are recognised. func NewPrinter(format string, quiet, noColor bool) *Printer { - name := strings.ToLower(format) + name := normalizeFormatName(format) var f Format switch name { case "", "table": From d1d7618423d1d556be6a368fad2b8ece516561c3 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 15:35:19 +0200 Subject: [PATCH 08/10] fix(plugin): address Copilot review round 3 - EXTENDING.md: fix Short description to match implementation (no path in Short) - EXTENDING.md: replace nonexistent stackctl config show with config get - EXTENDING.md: replace config-file parsing advice with config get commands - EXTENDING.md: fix bash error hint to suggest export instead of config set - README.md: clarify that plugins need explicitly exported env vars - output.go: RegisterFormat rejects empty/whitespace-only names Co-Authored-By: Claude Opus 4.6 --- EXTENDING.md | 27 ++++++++++----------------- README.md | 2 +- cli/pkg/output/output.go | 3 +++ 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/EXTENDING.md b/EXTENDING.md index 92d65ba..b3e8b6f 100644 --- a/EXTENDING.md +++ b/EXTENDING.md @@ -150,7 +150,7 @@ set -euo pipefail INSTANCE_ID=${1:?usage: stackctl snapshot-pvc } -: "${STACKCTL_API_URL:?STACKCTL_API_URL not set — run 'stackctl config set api-url ...' first}" +: "${STACKCTL_API_URL:?STACKCTL_API_URL not set — export it first, e.g. export STACKCTL_API_URL=\$(stackctl config get api-url)}" # Optional: allow insecure TLS per env CURL_OPTS=() @@ -313,7 +313,7 @@ Read `STACKCTL_API_URL` + `STACKCTL_API_KEY` from the environment. stackctl does Two workable strategies: 1. **Require the env vars**, and fail fast with a pointer (`export STACKCTL_API_URL=... STACKCTL_API_KEY=...` — or wrap your plugin in a shell function that exports them). -2. **Shell out to `stackctl config show -o json`** and parse the result. Works without any env wiring, at the cost of one extra exec. +2. **Shell out to `stackctl config get api-url`** and `stackctl config get api-key`. Works without any env wiring, at the cost of one extra exec per value. Either way, don't parse `~/.stackmanager/config.yaml` directly — the schema is internal and may change. @@ -382,18 +382,11 @@ On `Execute()`, stackctl scans every directory in `$PATH`. For each regular exec 2. Skip if a built-in subcommand with the same name is already registered. 3. Register a new Cobra subcommand: - `Use: ` - - `Short: "Plugin: ()"` - - `DisableFlagParsing: true` (plugin handles its own flags) + - `Short: "Plugin: "` (path kept out of the summary listing) + - `Long`: includes the plugin's absolute path for `stackctl help ` + - `DisableFlagParsing: true` — plugin handles its own flags - `RunE`: exec the binary with the remaining args, piping I/O, propagating exit code -Cobra subcommand registration is: - -- `Use: ` -- `Short: "Plugin: "` (path kept out of the summary listing) -- `Long`: includes the plugin's absolute path for `stackctl help ` -- `DisableFlagParsing: true` — plugin handles its own flags -- `RunE`: exec the binary with the remaining args, piping I/O, propagating exit code - If the user runs `stackctl …`, Cobra routes to the registered subcommand's `RunE`, which exec's the plugin. Source: [cli/cmd/plugins.go](cli/cmd/plugins.go) (≈110 lines). No plugin framework, no SDK — just `os/exec` + `$PATH`. @@ -423,12 +416,12 @@ Pick one workflow and document it for your plugin's users: stackctl my-plugin … ``` -- **Have the plugin fall back to reading the config file itself:** - parse `~/.stackmanager/config.yaml` and resolve the active context's - `api-url` + `api-key` if the env vars are empty. A ~30-line helper in - any language. +- **Have the plugin resolve config via `stackctl config get`:** + shell out to `stackctl config get api-url` and `stackctl config get api-key` + if the env vars are empty. One extra exec per value, but avoids parsing + internal config formats. -Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless you parse the config yourself. stackctl does **not** inject resolved config values when execing plugins. +Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless you resolve config via the commands above. ### Plugin exits non-zero but stackctl exits 0 diff --git a/README.md b/README.md index c6d0709..de5969a 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ stackctl --help | grep hello # hello Plugin: hello ``` -The plugin inherits `STACKCTL_API_URL`, `STACKCTL_API_KEY`, and the rest of the user's environment. Built-in subcommands always win on name collisions (a safety feature — a malicious `stackctl-config` on PATH can't intercept credentials). +The plugin inherits the user's full environment. If `STACKCTL_API_URL` and `STACKCTL_API_KEY` are exported in your shell, the plugin sees them. Values saved with `stackctl config set` are **not** automatically exported — export them yourself (`export STACKCTL_API_URL="$(stackctl config get api-url)"`) or see [EXTENDING.md](EXTENDING.md) for the full story. Built-in subcommands always win on name collisions (a safety feature — a malicious `stackctl-config` on PATH can't intercept credentials). 👉 **[Full guide: EXTENDING.md](EXTENDING.md)** — tutorial, recipes in bash/Python/Go, best practices, and how plugins pair with [server-side action webhooks](https://github.com/omattsson/k8s-stack-manager/blob/main/EXTENDING.md) for end-to-end custom operations. diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index b2dfaea..5156cd5 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -49,6 +49,9 @@ var ( // at startup rather than inside a render call that could be minutes later. func RegisterFormat(name string, fn FormatterFunc) { norm := normalizeFormatName(name) + if norm == "" { + panic("output: RegisterFormat called with empty/whitespace-only name") + } if norm == string(FormatTable) || norm == string(FormatJSON) || norm == string(FormatYAML) { panic(fmt.Sprintf("output: cannot override built-in format %q", norm)) } From 51c00435650f828fb32ae5245fb74510e581ccd6 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 15:47:45 +0200 Subject: [PATCH 09/10] fix(plugin): address Copilot review round 4 - output: RegisterSingleFormat rejects empty/whitespace-only names (matches RegisterFormat) - plugins: normalize STACKCTL_OUTPUT value before passing to subprocess - formatter_test: use unique format key to avoid parallel-test registry collision Co-Authored-By: Claude Opus 4.6 --- cli/cmd/plugins.go | 2 +- cli/pkg/output/formatter_test.go | 10 +++++----- cli/pkg/output/output.go | 3 +++ 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go index 1e1f113..846a2f8 100644 --- a/cli/cmd/plugins.go +++ b/cli/cmd/plugins.go @@ -133,7 +133,7 @@ func pluginEnv(cmd *cobra.Command) []string { } if flags.Changed("output") { if output, err := flags.GetString("output"); err == nil { - env = setEnv(env, "STACKCTL_OUTPUT", output) + env = setEnv(env, "STACKCTL_OUTPUT", strings.ToLower(strings.TrimSpace(output))) } } return env diff --git a/cli/pkg/output/formatter_test.go b/cli/pkg/output/formatter_test.go index f029212..429d63d 100644 --- a/cli/pkg/output/formatter_test.go +++ b/cli/pkg/output/formatter_test.go @@ -63,14 +63,14 @@ func TestRegisterFormat_CaseInsensitiveAndTrimmed(t *testing.T) { called++ return nil } - RegisterFormat(" CSV ", fn) - unregisterForTest(t, "csv") + RegisterFormat(" TRIMTEST ", fn) + unregisterForTest(t, "trimtest") - p := NewPrinter("csv", false, true) + p := NewPrinter("trimtest", false, true) p.Writer = io.Discard - assert.Equal(t, Format("csv"), p.Format, "NewPrinter should match case-insensitively") + assert.Equal(t, Format("trimtest"), p.Format, "NewPrinter should match case-insensitively") require.NoError(t, p.Print(nil, nil, nil, nil)) - assert.Equal(t, 1, called, "formatter registered under 'CSV' should be reached via 'csv'") + assert.Equal(t, 1, called, "formatter registered under 'TRIMTEST' should be reached via 'trimtest'") } // TestRegisterFormat_BuiltinCollisionIsCaseInsensitive ensures the diff --git a/cli/pkg/output/output.go b/cli/pkg/output/output.go index 5156cd5..275b767 100644 --- a/cli/pkg/output/output.go +++ b/cli/pkg/output/output.go @@ -72,6 +72,9 @@ func RegisterFormat(name string, fn FormatterFunc) { // Names are normalised identically to RegisterFormat. func RegisterSingleFormat(name string, fn SingleFormatterFunc) { norm := normalizeFormatName(name) + if norm == "" { + panic("output: RegisterSingleFormat called with empty/whitespace-only name") + } if fn == nil { panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", norm)) } From 35da51dbe188427aae3679343c37b6cccf6de244 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Sun, 19 Apr 2026 16:17:39 +0200 Subject: [PATCH 10/10] fix(plugin): address Copilot review round 5 - Prevent help/completion shadowing by external plugins - Case-insensitive .exe suffix stripping on Windows Co-Authored-By: Claude Opus 4.6 --- cli/cmd/plugins.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cli/cmd/plugins.go b/cli/cmd/plugins.go index 846a2f8..d4a05d5 100644 --- a/cli/cmd/plugins.go +++ b/cli/cmd/plugins.go @@ -39,6 +39,10 @@ var pluginNamePattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) // https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/ func registerPlugins(root *cobra.Command, pathEnv string) { builtins := existingCommandNames(root) + // Cobra registers "help" and "completion" implicitly — make sure a + // stackctl-help or stackctl-completion on PATH can't shadow them. + builtins["help"] = struct{}{} + builtins["completion"] = struct{}{} for name, path := range discoverPlugins(pathEnv) { if _, collides := builtins[name]; collides { continue @@ -78,7 +82,9 @@ func discoverPlugins(pathEnv string) map[string]string { } pluginName := strings.TrimPrefix(name, pluginPrefix) if runtime.GOOS == "windows" { - pluginName = strings.TrimSuffix(pluginName, ".exe") + if low := strings.ToLower(pluginName); strings.HasSuffix(low, ".exe") { + pluginName = pluginName[:len(pluginName)-4] + } } if !pluginNamePattern.MatchString(pluginName) { continue