feat(plugin): external command discovery; remove stack refresh-db#35
feat(plugin): external command discovery; remove stack refresh-db#35
Conversation
On Execute(), scan $PATH for executables named stackctl-<name> 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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 <id>` no longer exists. Install the stackctl-refresh-db plugin from kvk-devops-tools/refresh-db/ onto your PATH; `stackctl refresh-db <id>` 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.
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.
- 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 <plugin>` 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.
There was a problem hiding this comment.
Pull request overview
Adds external-command plugin discovery to extend stackctl via stackctl-<name> executables on $PATH, introduces a pluggable output-format registry, and removes the Klaravik-specific stack refresh-db command/client method in favor of a plugin-based workflow.
Changes:
- Discover and register external plugin commands at CLI startup, proxying execution with stdio passthrough and exit-code propagation.
- Add
pkg/outputregistries to support custom output formats (list + optional single-item formatter). - Remove
stack refresh-dbCobra command andClient.RefreshDBStack()(and associated tests/docs updates).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
cli/cmd/root.go |
Registers external plugins during Execute() before Cobra dispatch. |
cli/cmd/plugins.go |
Implements PATH scanning, collision handling, env forwarding, and plugin command wrapper. |
cli/cmd/plugins_test.go |
Adds tests for plugin discovery/registration and I/O passthrough behavior. |
cli/cmd/stack.go |
Removes stack refresh-db subcommand and its flag wiring/registration. |
cli/cmd/stack_test.go |
Removes tests covering the deleted stack refresh-db command. |
cli/pkg/client/client.go |
Removes RefreshDBStack() API method. |
cli/pkg/client/client_test.go |
Removes tests for the deleted RefreshDBStack() method. |
cli/pkg/output/output.go |
Adds custom formatter registries and routes printer output through them. |
cli/pkg/output/formatter_test.go |
Adds unit tests for formatter registration/routing behavior. |
README.md |
Documents plugin mechanism and a quick “hello” example. |
EXTENDING.md |
Adds plugin authoring guide, recipes, and troubleshooting details. |
| full := filepath.Join(dir, name) | ||
| info, err := os.Stat(full) | ||
| if err != nil || !info.Mode().IsRegular() { | ||
| continue | ||
| } |
There was a problem hiding this comment.
discoverPlugins stores full := filepath.Join(dir, name) without ensuring it’s absolute. If a user has relative PATH entries, this can produce a relative binaryPath, contradicting the comment/nolint about an “absolute path captured at discovery time” and making execution dependent on the current working directory. Consider resolving full to an absolute (and optionally symlink-evaluated) path before storing/execing it.
|
|
||
| stackctl hello world # → Hello! API=http://... args=world | ||
| stackctl --help | grep hello | ||
| # hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) |
There was a problem hiding this comment.
The example stackctl --help | grep hello output includes the plugin’s absolute path, but newPluginCommand intentionally hides the path from the Short help text (it’s only in Long). Update the example output to match what the CLI now prints.
| # hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) | |
| # hello Plugin: hello |
|
|
||
| ### Arguments | ||
|
|
||
| Whatever the user typed after `stackctl <name>`. `stackctl --help <name>` shows the plugin in the command list; `stackctl <name> --help` is delegated to the plugin's own help. |
There was a problem hiding this comment.
stackctl --help <name> is not a typical Cobra help invocation (the --help flag generally ignores extra args). Prefer documenting stackctl help <name> for detailed help, and stackctl <name> --help for plugin-delegated help.
| Whatever the user typed after `stackctl <name>`. `stackctl --help <name>` shows the plugin in the command list; `stackctl <name> --help` is delegated to the plugin's own help. | |
| Whatever the user typed after `stackctl <name>`. Use `stackctl help <name>` for detailed built-in help about a named command; `stackctl <name> --help` is delegated to the plugin's own help. |
| // any Printer is used concurrently — the registry is read-locked on every | ||
| // render so adds during rendering are racy. |
There was a problem hiding this comment.
The comment says “adds during rendering are racy”, but the registry is protected by an RWMutex, so concurrent registration/rendering is race-free from a Go data-race perspective. If the intent is “unsupported / may lead to inconsistent output”, please reword to avoid implying an actual data race.
| // any Printer is used concurrently — the registry is read-locked on every | |
| // render so adds during rendering are racy. | |
| // any Printer is used concurrently. The registry is synchronized, so | |
| // registering while rendering is not a Go data race, but doing so is | |
| // unsupported and may lead to inconsistent behavior across concurrent renders. |
| 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)) | ||
| } |
There was a problem hiding this comment.
NewPrinter normalizes format names to lowercase, but RegisterFormat/RegisterSingleFormat store keys as-is and the built-in collision check is case-sensitive. This can make registered formats unreachable (e.g. RegisterFormat("CSV", ...)), and can lead to surprising duplicates. Consider normalizing name (e.g., strings.ToLower(strings.TrimSpace(name))) consistently in registration and lookups, and performing built-in collision checks on the normalized value.
|
|
||
| ```bash | ||
| stackctl --help | grep hello | ||
| # → hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) |
There was a problem hiding this comment.
This example help output includes the plugin’s absolute path, but the implementation sets Short: "Plugin: <name>" specifically to avoid leaking paths in --help output. Update the example output (or change the implementation) so docs and behavior match.
| # → hello Plugin: hello (/Users/you/.local/bin/stackctl-hello) | |
| # → hello Plugin: hello |
| ### 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 | |
There was a problem hiding this comment.
Docs claim plugins receive STACKCTL_API_URL/STACKCTL_API_KEY from “user env / stackctl config”, but the plugin env injection currently only forwards the existing process environment plus a few flag-derived vars (it doesn’t export resolved config values). Either update the docs to say these must already be present in the environment, or update plugin execution to inject resolved API URL/key from flags/env/config.
| ### 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 | | |
| ### Environment variables (inherited + flag-derived) | |
| The plugin inherits **stackctl's entire environment** (same pattern as `git`, | |
| `kubectl`, `gh`). In addition, stackctl exports a small set of variables | |
| derived from active CLI flags before `exec` so plugins can observe the user's | |
| requested output and TLS behavior. stackctl does **not** inject values resolved | |
| from `stackctl config`; if your plugin needs `STACKCTL_API_URL` or | |
| `STACKCTL_API_KEY`, they must already be present in the environment when | |
| `stackctl` is launched. | |
| | Variable | Source | Purpose | | |
| |---|---|---| | |
| | `STACKCTL_API_URL` | parent shell environment | Base URL of the k8s-stack-manager API, if already exported | | |
| | `STACKCTL_API_KEY` | parent shell environment | API key (header `X-API-Key`), if already exported | | |
| | `STACKCTL_INSECURE` | `--insecure` flag | `1` to skip TLS verification | |
| 2. Skip if a built-in subcommand with the same name is already registered. | ||
| 3. Register a new Cobra subcommand: | ||
| - `Use: <name>` | ||
| - `Short: "Plugin: <name> (<absolute-path>)"` |
There was a problem hiding this comment.
Internal docs say the plugin is registered with Short: "Plugin: <name> (<absolute-path>)", but the code uses Short: "Plugin: <name>" and keeps the path only in Long. Update this section to match the actual command registration.
| - `Short: "Plugin: <name> (<absolute-path>)"` | |
| - `Short: "Plugin: <name>"` | |
| - `Long`: includes the plugin's absolute path |
| The user hasn't configured it. Either: | ||
| - They should `stackctl config set api-url <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. | ||
|
|
There was a problem hiding this comment.
Troubleshooting suggests stackctl config set api-url <url> then “re-export the env var”, but stackctl config is stored in ~/.stackmanager/config.yaml and does not automatically export STACKCTL_API_URL into the environment. Please clarify the expected workflow (e.g., explicitly export STACKCTL_API_URL=..., or recommend plugins read the config file, or have stackctl inject resolved config values when execing plugins).
| The user hasn't configured it. Either: | |
| - They should `stackctl config set api-url <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. | |
| The plugin process only sees environment variables that are already exported in the user's shell. `stackctl config set api-url <url>` writes `~/.stackmanager/config.yaml`; it does **not** export `STACKCTL_API_URL` for plugins. | |
| Pick one workflow and document it for your plugin users: | |
| - If your plugin expects env vars, have the user explicitly export them, for example: `export STACKCTL_API_URL="$(stackctl config get api-url)"` (and similarly `export STACKCTL_API_KEY=...` if needed) | |
| - Or have your plugin fall back to reading `~/.stackmanager/config.yaml` and resolving the active context's `api-url` itself | |
| Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so they only inherit the caller's environment unless they parse the config themselves. `stackctl` does not inject resolved config values when execing plugins. |
| name := entry.Name() | ||
| if !strings.HasPrefix(name, pluginPrefix) || name == pluginPrefix { | ||
| continue | ||
| } | ||
| pluginName := strings.TrimPrefix(name, pluginPrefix) | ||
| if _, seen := found[pluginName]; seen { |
There was a problem hiding this comment.
discoverPlugins accepts any suffix after stackctl- as a Cobra command name. If the filename contains whitespace or starts with - (e.g. stackctl--help), Cobra parsing/help output can break or route unexpectedly. Consider validating pluginName against the documented allowed set (letters/digits/dashes) and skipping invalid names.
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 <name>` (not a real Cobra invocation) to `help <name>` (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.
| 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") | ||
| } |
There was a problem hiding this comment.
pluginEnv only appends STACKCTL_INSECURE=1 when the flag is true. If the user already has STACKCTL_INSECURE=1 in their environment and explicitly runs with --insecure=false, the plugin will still see insecure mode enabled (because the original env var is preserved). If the intent is "flag > env", consider checking whether the flag was explicitly set (PersistentFlags().Changed) and then forcing STACKCTL_INSECURE=0/1 accordingly.
| cat <<MSG | ||
| Hello from a stackctl plugin! | ||
| API URL: ${STACKCTL_API_URL:-<not set>} | ||
| API key: ${STACKCTL_API_KEY:+***configured***}${STACKCTL_API_KEY:-<not set>} |
There was a problem hiding this comment.
The tutorial snippet prints the API key value when STACKCTL_API_KEY is set: ${STACKCTL_API_KEY:+***configured***}${STACKCTL_API_KEY:-<not set>} expands to ***configured***<actual-key>. This encourages leaking credentials into terminals/logs. Update the example to never echo the key (only print a redacted marker like "configured" or "").
| API key: ${STACKCTL_API_KEY:+***configured***}${STACKCTL_API_KEY:-<not set>} | |
| API key: $( [ -n "${STACKCTL_API_KEY:-}" ] && printf '%s' '***configured***' || printf '%s' '<not set>' ) |
| ### 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 <url>`) — don't try to parse `~/.stackmanager/config.yaml` yourself. | ||
|
|
There was a problem hiding this comment.
“Don’t re-implement auth” says that if a stackctl context is configured, STACKCTL_API_URL/STACKCTL_API_KEY “pass through”, but earlier/later the guide states stackctl does NOT inject config-resolved values into the plugin env (only parent-shell env + flag-derived vars). This is contradictory and can mislead plugin authors; please align the wording (e.g., require explicit export or suggest stackctl config get ... export helpers).
|
|
||
| **Good plugin candidates:** | ||
|
|
||
| - **Thin wrappers around actions.** `stackctl refresh-db <id>` 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. |
There was a problem hiding this comment.
The guide states stackctl refresh-db <id> POSTs to /api/v1/stack-instances/:id/actions/refresh-db, but there are no /actions/ endpoints referenced anywhere in this codebase (client methods currently don’t use that path pattern). If this is meant as a server-side extension example, it would help to phrase it as “could POST to … (if your backend exposes it)” or otherwise ensure the documented endpoint matches the actual API/plugin you’re migrating to.
| - **Thin wrappers around actions.** `stackctl refresh-db <id>` 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 actions.** `stackctl refresh-db <id>` could POST to `/api/v1/stack-instances/:id/actions/refresh-db` **if your backend exposes that endpoint**, then pretty-print the result. This pairs well with a webhook/action handler registered on the server side. |
| func NewPrinter(format string, quiet, noColor bool) *Printer { | ||
| f := FormatTable | ||
| switch strings.ToLower(format) { | ||
| name := strings.ToLower(format) |
There was a problem hiding this comment.
NewPrinter normalizes the format name with strings.ToLower only, but the registry uses normalizeFormatName (TrimSpace + ToLower). This means values like " csv " (or other whitespace) won't match a registered custom formatter even though the registry contract is whitespace-tolerant. Consider using normalizeFormatName(format) here (and in the switch) so built-ins and custom formats are resolved consistently.
| name := strings.ToLower(format) | |
| name := normalizeFormatName(format) |
| // 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. |
There was a problem hiding this comment.
pluginNamePattern is documented as allowing ASCII letters/digits/dashes, but the regex only permits lowercase a-z. If uppercase plugin names should be supported, adjust the regex/normalization; if lowercase-only is intentional, update the comment/docs to make that constraint explicit so authors don’t get surprised by plugins being silently skipped.
| // 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 so they form valid Cobra command names. Names with | |
| // whitespace, uppercase letters, or leading dashes (for example | |
| // "stackctl- bad", "stackctl-Foo", or "stackctl--help") break help | |
| // routing and are skipped at discovery time. |
| 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) |
There was a problem hiding this comment.
Similar precedence issue for quiet/output: pluginEnv only sets STACKCTL_QUIET/STACKCTL_OUTPUT when the corresponding flag is true/non-empty, but it never overrides an existing env var when the user explicitly passes --quiet=false or --output=table (etc.). If these variables are meant to reflect the effective flag values, consider using PersistentFlags().Changed to decide when to force an explicit value into the plugin environment.
| 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) | |
| func setOrAppendEnv(env []string, key, value string) []string { | |
| prefix := key + "=" | |
| entry := prefix + value | |
| for i, kv := range env { | |
| if strings.HasPrefix(kv, prefix) { | |
| env[i] = entry | |
| return env | |
| } | |
| } | |
| return append(env, entry) | |
| } | |
| func pluginEnv(cmd *cobra.Command) []string { | |
| env := os.Environ() | |
| // Resolve flags with explicit CLI precedence for plugin commands. The | |
| // config layer may not be loaded yet because persistent pre-run depends on | |
| // the command name, so when a persistent flag was explicitly set we force | |
| // that effective value into the plugin environment, including false/default | |
| // values that must override inherited STACKCTL_* vars. | |
| if cmd != nil { | |
| flags := cmd.Root().PersistentFlags() | |
| if flag := flags.Lookup("insecure"); flag != nil && flag.Changed { | |
| if insecure, err := flags.GetBool("insecure"); err == nil { | |
| if insecure { | |
| env = setOrAppendEnv(env, "STACKCTL_INSECURE", "1") | |
| } else { | |
| env = setOrAppendEnv(env, "STACKCTL_INSECURE", "0") | |
| } | |
| } | |
| } | |
| if flag := flags.Lookup("quiet"); flag != nil && flag.Changed { | |
| if quiet, err := flags.GetBool("quiet"); err == nil { | |
| if quiet { | |
| env = setOrAppendEnv(env, "STACKCTL_QUIET", "1") | |
| } else { | |
| env = setOrAppendEnv(env, "STACKCTL_QUIET", "0") | |
| } | |
| } | |
| } | |
| if flag := flags.Lookup("output"); flag != nil && flag.Changed { | |
| if output, err := flags.GetString("output"); err == nil { | |
| env = setOrAppendEnv(env, "STACKCTL_OUTPUT", output) | |
| } |
| func RegisterSingleFormat(name string, fn SingleFormatterFunc) { | ||
| norm := normalizeFormatName(name) | ||
| if fn == nil { | ||
| panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", norm)) | ||
| } |
There was a problem hiding this comment.
RegisterSingleFormat’s doc says it’s for an already-registered format, but the implementation doesn’t verify that a corresponding list formatter exists in formatRegistry. This can lead to silent misconfiguration (NewPrinter won’t select the format, so the single formatter is effectively unreachable). Either enforce the precondition (panic/return error when the base format isn’t registered) or update the doc/behavior accordingly.
| // Windows: .exe suffix handling via PATHEXT is honoured; the POSIX | ||
| // executable-bit check is skipped because Windows represents executability | ||
| // via extension, not permission bits. |
There was a problem hiding this comment.
The comment claims "Windows: .exe suffix handling via PATHEXT is honoured", but the code only trims a literal ".exe" and does not consult PATHEXT (so .cmd/.bat/.com plugins won’t be discovered). Either implement PATHEXT-based suffix handling or adjust the comment to match the actual behavior.
| // 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: only a literal .exe suffix is trimmed from discovered plugin | |
| // names; the POSIX executable-bit check is skipped because Windows | |
| // represents executability via extension, not permission bits. |
- 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) <noreply@anthropic.com>
| 2. Skip if a built-in subcommand with the same name is already registered. | ||
| 3. Register a new Cobra subcommand: | ||
| - `Use: <name>` | ||
| - `Short: "Plugin: <name> (<absolute-path>)"` |
There was a problem hiding this comment.
In the internal "How it works" list, Short is described as including the absolute path, but the implementation intentionally keeps the path out of Short and puts it in Long. Please align this bullet list with the actual registration behavior to avoid confusing plugin authors.
| - `Short: "Plugin: <name> (<absolute-path>)"` | |
| - `Short: "Plugin: <name>"` (path kept out of the summary listing) | |
| - `Long`: includes the plugin's absolute path for `stackctl help <name>` |
| - **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. stackctl does **not** inject resolved config values when execing plugins. |
There was a problem hiding this comment.
This troubleshooting advice recommends parsing ~/.stackmanager/config.yaml directly, which contradicts the earlier guidance to avoid parsing the config file because the schema may change. Consider recommending shelling out to stackctl config get ... (or documenting a stable machine-readable command) instead of reading the YAML directly.
| - **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. stackctl does **not** inject resolved config values when execing plugins. | |
| - **Have the plugin resolve config by shelling out to `stackctl`:** | |
| if the env vars are empty, run `stackctl config get api-url` and | |
| `stackctl config get api-key` from the plugin and use those results. | |
| This avoids depending on the internal `~/.stackmanager/config.yaml` | |
| schema, which may change. | |
| Core stackctl commands do config resolution automatically; plugins are plain exec'd subprocesses, so env is all they get unless they explicitly resolve config themselves. stackctl does **not** inject resolved config values when execing plugins. |
| # 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). |
There was a problem hiding this comment.
The quick example/output implies STACKCTL_API_URL will be populated after configuring stackctl, but plugins only inherit already-exported environment variables. Consider adding a short note here that users must export STACKCTL_API_URL/STACKCTL_API_KEY (or follow EXTENDING.md’s workflow) for plugins to see them.
| 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 exported environment variables such as `STACKCTL_API_URL`, `STACKCTL_API_KEY`, and the rest of the user's environment. Note that values saved with `stackctl config set` are not automatically exported to your shell, so if you want plugins to see them you must export `STACKCTL_API_URL` / `STACKCTL_API_KEY` yourself (or follow the workflow in `EXTENDING.md`). Built-in subcommands always win on name collisions (a safety feature — a malicious `stackctl-config` on PATH can't intercept credentials). |
| 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, err := filepath.Abs(filepath.Join(dir, name)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| info, err := os.Stat(full) | ||
| if err != nil || !info.Mode().IsRegular() { | ||
| continue | ||
| } | ||
| if runtime.GOOS != "windows" && info.Mode().Perm()&0o111 == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
On Windows, this will accept stackctl-foo files without a .exe suffix as plugins (since only the name is stripped), but exec.Command typically can't run extensionless files. This can lead to registering non-runnable plugins and confusing runtime errors. Consider requiring .exe on Windows (or using exec.LookPath/PATHEXT-aware resolution) and skipping entries that aren't actually runnable.
| 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()) | ||
| } |
There was a problem hiding this comment.
The os.Exit(exitErr.ExitCode()) path (exit-code propagation) isn't covered by tests right now, but it’s a key part of the plugin contract. Consider adding a test that runs a failing plugin via a subprocess (e.g., invoking a small helper test binary) and asserts stackctl exits with the plugin’s exact exit code.
| func RegisterFormat(name string, fn FormatterFunc) { | ||
| 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", norm)) | ||
| } |
There was a problem hiding this comment.
RegisterFormat allows an empty/whitespace-only name (normalizes to ""), which can put a formatter under the empty key and makes it impossible to select via --output (since empty currently maps to table). Consider rejecting empty names with a panic/error to avoid silent misconfiguration.
|
|
||
| INSTANCE_ID=${1:?usage: stackctl snapshot-pvc <instance-id>} | ||
|
|
||
| : "${STACKCTL_API_URL:?STACKCTL_API_URL not set — run 'stackctl config set api-url ...' first}" |
There was a problem hiding this comment.
This recipe suggests running stackctl config set api-url ... will satisfy STACKCTL_API_URL, but plugins only see exported environment variables (not values stored in ~/.stackmanager/config.yaml). The hint should instead tell users to export STACKCTL_API_URL (e.g., export STACKCTL_API_URL="$(stackctl config get api-url)") or have the plugin shell out to stackctl config get.
| : "${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)\"}" |
| 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. |
There was a problem hiding this comment.
The doc references stackctl config show -o json, but there is no config show subcommand in this repo (available commands are set/get/list/...). Update this to a real command (e.g., stackctl config get api-url / stackctl config get api-key) so plugin authors don’t copy/paste a non-existent invocation.
| 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 real config commands** such as `stackctl config get api-url` and `stackctl config get api-key`, then capture those values individually. Works without any env wiring, at the cost of one extra exec per lookup. |
- 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 <noreply@anthropic.com>
| func RegisterSingleFormat(name string, fn SingleFormatterFunc) { | ||
| norm := normalizeFormatName(name) | ||
| if fn == nil { | ||
| panic(fmt.Sprintf("output: nil SingleFormatterFunc for format %q", norm)) | ||
| } | ||
| 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)) | ||
| } |
There was a problem hiding this comment.
RegisterSingleFormat doesn’t validate an empty/whitespace-only format name (unlike RegisterFormat). With name=="" (or built-in names), the panic message currently misleadingly reports an ordering issue ("called before RegisterFormat") instead of a bad name. Add the same norm == "" guard (and ideally the built-in-name guard) as RegisterFormat so registration failures are explicit and consistent.
| 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)) |
There was a problem hiding this comment.
This test runs with t.Parallel() and registers the format name csv, which is also used by TestNewPrinter_UsesCustomFormat. Because the registries are package-global, the two parallel tests can overwrite/delete each other’s registration, causing flaky failures. Use a unique format name per test (e.g. csv_case_insensitive) or remove t.Parallel() for the tests that share registry keys.
| 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") |
There was a problem hiding this comment.
This test registers the custom format name csv while running in parallel. TestRegisterFormat_CaseInsensitiveAndTrimmed also registers/cleans up the same csv key, so the two tests can interfere via the shared global registries and become flaky. Rename one of the formats to a unique key or avoid t.Parallel() for registry-mutating tests.
| } | ||
| } | ||
| if flags.Changed("output") { | ||
| if output, err := flags.GetString("output"); err == nil { |
There was a problem hiding this comment.
STACKCTL_OUTPUT is exported to plugins using the raw --output flag value. Since stackctl itself normalizes output names case-insensitively (and trims whitespace) in output.NewPrinter, a user running --output JSON will get JSON output but plugins will see STACKCTL_OUTPUT=JSON, which is inconsistent. Normalize the value before exporting (lowercase + trim) so plugins see the effective format key stackctl will use.
| if output, err := flags.GetString("output"); err == nil { | |
| if output, err := flags.GetString("output"); err == nil { | |
| output = strings.ToLower(strings.TrimSpace(output)) |
- 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 <noreply@anthropic.com>
| 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{}{} | ||
| } |
There was a problem hiding this comment.
Plugin collision protection currently only considers commands already present in root.Commands(). Since stackctl doesn’t explicitly register Cobra’s default help command before calling registerPlugins(), a stackctl-help executable on PATH can be registered as stackctl help, effectively shadowing the built-in help UX. Consider reserving names like help (and potentially other Cobra-reserved commands) or explicitly initializing the default help command before computing existingCommandNames() so plugins cannot override it.
| } | ||
| pluginName := strings.TrimPrefix(name, pluginPrefix) | ||
| if runtime.GOOS == "windows" { | ||
| pluginName = strings.TrimSuffix(pluginName, ".exe") |
There was a problem hiding this comment.
On Windows, discovery strips only a lowercase .exe suffix. A plugin built/renamed with an uppercase .EXE (common on Windows filesystems) will not have the suffix removed and will fail the pluginNamePattern check, so it won’t be discovered. Consider using a case-insensitive .exe check/trim (e.g., via strings.EqualFold on filepath.Ext) before validating the plugin name.
| pluginName = strings.TrimSuffix(pluginName, ".exe") | |
| if ext := filepath.Ext(pluginName); strings.EqualFold(ext, ".exe") { | |
| pluginName = pluginName[:len(pluginName)-len(ext)] | |
| } |
| 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{}{} | ||
| } |
There was a problem hiding this comment.
registerPlugins iterates over the map returned by discoverPlugins; Go map iteration order is randomized, so the order plugins are added (and thus shown in stackctl --help) will be nondeterministic across runs. To keep help output stable, collect plugin names into a slice, sort it, then add commands in sorted order.
| pluginName := strings.TrimPrefix(name, pluginPrefix) | ||
| if runtime.GOOS == "windows" { | ||
| pluginName = strings.TrimSuffix(pluginName, ".exe") | ||
| } | ||
| if !pluginNamePattern.MatchString(pluginName) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
On Windows, trimming the .exe suffix is case-sensitive (strings.TrimSuffix(pluginName, ".exe")), so a plugin named stackctl-foo.EXE (valid on Windows) will not be discovered because the suffix isn't removed and the regex check will fail. Consider normalizing the filename case for the extension check or using a case-insensitive .exe match.
| func TestRegisterPlugins_PluginInvocationPassesThroughArgsAndStdout(t *testing.T) { | ||
|
|
||
| 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)) | ||
| } |
There was a problem hiding this comment.
This test claims to verify the Cobra wrapper passes args/stdout through, but it executes the plugin binary directly via exec.Command(bin, ...) instead of invoking pluginCmd.RunE. As written, it doesn't exercise newPluginCommand's passthrough wiring; consider calling the registered Cobra command (and capturing root's output) or removing the redundant assertion.
- Prevent help/completion shadowing by external plugins - Case-insensitive .exe suffix stripping on Windows Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| func pluginEnv(cmd *cobra.Command) []string { | ||
| env := os.Environ() | ||
| 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 flags.Changed("quiet") { | ||
| if quiet, err := flags.GetBool("quiet"); err == nil { | ||
| env = setEnv(env, "STACKCTL_QUIET", boolEnvValue(quiet)) | ||
| } | ||
| } | ||
| if flags.Changed("output") { | ||
| if output, err := flags.GetString("output"); err == nil { | ||
| env = setEnv(env, "STACKCTL_OUTPUT", strings.ToLower(strings.TrimSpace(output))) | ||
| } | ||
| } | ||
| return env |
There was a problem hiding this comment.
pluginEnv only injects STACKCTL_QUIET/STACKCTL_OUTPUT when the corresponding persistent flags were explicitly set (Flags().Changed). This means plugins may (a) see no STACKCTL_OUTPUT at all when the user relies on the default "table", and (b) accidentally inherit STACKCTL_QUIET/STACKCTL_OUTPUT from the parent shell even when the user did not request quiet/custom output for this invocation—diverging from stackctl’s own flag behavior. Consider always setting STACKCTL_QUIET and STACKCTL_OUTPUT from the resolved flag values for each invocation (and only preserving an existing env var for STACKCTL_INSECURE if that’s an intentional compatibility feature).
| // 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) { | ||
|
|
||
| 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") | ||
| } |
There was a problem hiding this comment.
TestRegisterPlugins_RunViaCobraRouting doesn’t currently exercise Cobra’s routing/Execute path as the comment describes: it manually finds the command and calls RunE, and the root.SetArgs(...) setup is unused. Either rename/adjust the test to reflect what it actually verifies (arg passthrough via RunE), or rework it to run root.Execute() in a way that can assert routing without terminating the test process.
| 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 | ||
| } | ||
| root.AddCommand(newPluginCommand(name, path)) | ||
| builtins[name] = struct{}{} | ||
| } |
There was a problem hiding this comment.
registerPlugins iterates over a map returned by discoverPlugins. Go map iteration order is randomized, so the order that plugin subcommands are added (and therefore the order they appear in stackctl --help) will be nondeterministic between runs. Consider collecting plugin names, sorting them, and then adding commands in sorted order for stable help output.
Summary
Adds external-command plugin discovery so users can extend stackctl with any executable named
stackctl-<name>on their$PATH— same pattern asgit,kubectl, andgh. Removes the Klaravik-specificstack refresh-dbsubcommand; it now ships as a plugin in kvk-devops-tools.New
cli/cmd/plugins.go— PATH-scan atExecute(), first-win semantics, built-in collision protection, exit-code propagation, stdin/stdout/stderr pass-throughcli/pkg/output/— pluggable output format registry (RegisterFormat/RegisterSingleFormat) alongside the built-intable/json/yamlSTACKCTL_INSECURE,STACKCTL_QUIET,STACKCTL_OUTPUTresolved from flags, plus the parent environment'sSTACKCTL_API_URL/STACKCTL_API_KEYRemoved (BREAKING)
stackctl stack refresh-db <id>subcommandClient.RefreshDBStack()frompkg/clientMigration: install the
stackctl-refresh-dbplugin from the kvk-devops-tools repo onto your$PATH;stackctl refresh-db <id>then becomes available via external-command discovery. The subcommand is no longer nested understack.Reviewer notes
Code review cycle completed before this PR:
--helplistings (was leaking$HOMEin screenshots); still visible instackctl help <plugin>Long textFormatterFuncrejected at registration (not at render time)HTTPError/URLErrorhandling + stderr warning on insecure modeTest coverage
cli/cmd/plugins_test.go— 10 tests: discovery, skip-non-executable, first-PATH-wins, subcommand registration, built-in collision (with count assertion), arg passthrough, Cobra routing, stdin passthrough, no-op on empty PATHcli/pkg/output/formatter_test.go— 9 tests: panic-on-built-in-override, panic-on-nil-func, custom format routing, single-formatter fallback, concurrent readsgo vet ./...cleanTest plan
go test ./... -count=1— all passgo vet ./...— cleaninstall stackctl-refresh-db ~/.local/bin/; runstackctl --help— plugin appears, no path in listingstackctl refresh-db <id>against a live backend — full round-trip verified🤖 Generated with Claude Code