Phase 1.1: Scaffold CLI module, config, and HTTP client#11
Conversation
Implement the Phase 1.1 foundation for stackctl: - Go module with Cobra commands and Viper-based config - Named contexts with config set/get/list/use-context/delete-context - HTTP client with dual auth (JWT + API key), error mapping - Client-side types for all API resources - Output formatters (table, JSON, YAML, quiet mode) with colored status - Token storage with 0600 permissions and expiry detection - Version command with build-time ldflags Tests (93 total, 78.3% coverage): - Unit tests for pkg/config, pkg/client, pkg/output, cmd/ - Integration tests for config and token filesystem workflows - E2E tests building and running the actual binary Closes #1 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR scaffolds the initial stackctl Go CLI, adding configuration/context management, a basic HTTP API client, output formatting helpers, and a comprehensive unit/integration/e2e test suite to validate core workflows.
Changes:
- Added CLI entrypoint and Cobra command structure (
root,config,version) plus token file helpers. - Implemented config persistence (contexts, current-context, token paths) and an HTTP JSON client with API error mapping.
- Introduced output formatting utilities (table/json/yaml/quiet primitives) and added unit/integration/e2e tests.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/.gitignore | Ignores built binary output and coverage file. |
| cli/go.mod | Defines the Go module and dependencies. |
| cli/go.sum | Adds dependency checksums for the new module. |
| cli/main.go | CLI entrypoint; wires build-time version info and executes root command. |
| cli/cmd/root.go | Root command, global flags, config loading, and client construction helpers. |
| cli/cmd/config.go | Implements stackctl config subcommands for contexts and key/value settings. |
| cli/cmd/config_test.go | Unit tests for config subcommands. |
| cli/cmd/token.go | Token persistence helpers (save/load/delete) for current context. |
| cli/cmd/token_test.go | Unit tests for token helpers (permissions, expiry behavior, round-trip). |
| cli/cmd/version.go | Implements stackctl version with optional JSON output. |
| cli/cmd/version_test.go | Unit tests for version output and build info wiring. |
| cli/pkg/config/config.go | Config model + load/save + context key get/set + path helpers. |
| cli/pkg/config/config_test.go | Unit tests for config parsing, persistence, and helper functions. |
| cli/pkg/client/client.go | HTTP client wrapper with auth headers, request helpers, and APIError mapping. |
| cli/pkg/client/client_test.go | Unit tests for auth behavior, HTTP methods, query params, and error handling. |
| cli/pkg/output/output.go | Output printer supporting table/json/yaml plus status coloring helpers. |
| cli/pkg/output/output_test.go | Unit tests for printer formats, quiet mode, and table/single rendering. |
| cli/pkg/types/types.go | Adds client-side resource/type definitions used by the API client. |
| cli/test/integration/config_integration_test.go | Integration tests for config + token filesystem behavior and permissions. |
| cli/test/e2e/cli_e2e_test.go | E2E tests that build and execute the stackctl binary for key flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // JWT token from stored token file (only if no API key) | ||
| if c.APIKey == "" { | ||
| token, _ := loadToken() | ||
| c.Token = token | ||
| } |
There was a problem hiding this comment.
The error from loadToken() is intentionally ignored here. If loadToken() returns an expiry/parse error, the CLI will proceed unauthenticated with no user feedback. Consider handling the error and surfacing a helpful message (or treating expired tokens as empty without error).
| func main() { | ||
| cmd.SetVersionInfo(version, commit, date) | ||
| if err := cmd.Execute(); err != nil { | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
With rootCmd.SilenceErrors = true and main() exiting on error without printing it, user-facing errors will be completely silent (exit code 1, no stderr message). Either disable SilenceErrors or print the returned error in main (e.g., to stderr) before exiting so users can diagnose failures.
| if err := cfg.Save(); err != nil { | ||
| return err | ||
| } | ||
| printer.PrintMessage("Set %s = %s in context %q", key, value, cfg.CurrentContext) |
There was a problem hiding this comment.
config set echoes the raw value back to stdout (Set %s = %s ...). When setting api-key, this will print the full secret to the terminal (and potentially logs/shell scrollback). Consider masking/redacting sensitive keys (e.g., show only last 4 chars) or printing a generic confirmation for secrets.
| printer.PrintMessage("Set %s = %s in context %q", key, value, cfg.CurrentContext) | |
| displayValue := value | |
| if key == "api-key" { | |
| // Mask API key in confirmation message, show only last 4 chars | |
| if len(value) > 4 { | |
| displayValue = "***" + value[len(value)-4:] | |
| } else { | |
| displayValue = "***" | |
| } | |
| } | |
| printer.PrintMessage("Set %s = %s in context %q", key, displayValue, cfg.CurrentContext) |
| // ConfigDir returns the configuration directory path. | ||
| // Uses STACKCTL_CONFIG_DIR env var if set, otherwise ~/.stackmanager. | ||
| func ConfigDir() (string, error) { | ||
| if dir := os.Getenv("STACKCTL_CONFIG_DIR"); dir != "" { | ||
| return dir, nil | ||
| } | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", fmt.Errorf("determining home directory: %w", err) | ||
| } | ||
| return filepath.Join(home, DefaultConfigDir), nil | ||
| } |
There was a problem hiding this comment.
ConfigDir currently always defaults to ~/.stackmanager when STACKCTL_CONFIG_DIR is unset, but the PR description/issue mention XDG-aware config paths. If XDG support is required, update this to respect XDG_CONFIG_HOME (and/or OS-specific config dirs) and adjust tests accordingly.
| switch key { | ||
| case "api-url": | ||
| ctx.APIURL = value | ||
| case "api-key": | ||
| ctx.APIKey = value | ||
| case "insecure": | ||
| ctx.Insecure = value == "true" | ||
| default: | ||
| return fmt.Errorf("unknown config key: %s", key) | ||
| } |
There was a problem hiding this comment.
SetContextValue silently treats any value other than the exact string "true" as false for the insecure key. This can lead to surprising behavior (e.g., "True", "1", or typos) without an error. Consider parsing with strconv.ParseBool and returning a validation error on invalid inputs.
| func newClient() (*client.Client, error) { | ||
| apiURL := resolveAPIURL() | ||
| if apiURL == "" { | ||
| return nil, errNoAPIURL | ||
| } | ||
|
|
||
| c := client.New(apiURL) |
There was a problem hiding this comment.
The config supports an insecure setting, but newClient() doesn't propagate it to the API client (and there is no flag/env override for it). This makes stackctl config set insecure true a no-op. Wire cfg.CurrentCtx().Insecure (and/or a global --insecure flag) into the client and ensure the HTTP transport honors it (TLS verification skip).
| // loadToken reads the JWT token for the current context. | ||
| // Returns empty string if no token exists or token is expired. | ||
| func loadToken() (string, error) { | ||
| if cfg.CurrentContext == "" { | ||
| return "", nil | ||
| } | ||
|
|
||
| path, err := config.TokenPath(cfg.CurrentContext) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return "", nil | ||
| } | ||
| return "", fmt.Errorf("reading token file: %w", err) | ||
| } | ||
|
|
||
| var t storedToken | ||
| if err := json.Unmarshal(data, &t); err != nil { | ||
| return "", fmt.Errorf("parsing token file: %w", err) | ||
| } | ||
|
|
||
| if !t.ExpiresAt.IsZero() && time.Now().After(t.ExpiresAt) { | ||
| return "", fmt.Errorf("token expired. Run 'stackctl login' to re-authenticate") | ||
| } |
There was a problem hiding this comment.
loadToken's comment says it returns an empty string if the token is expired, but the implementation returns a non-nil error when expired. Either update the comment to match the behavior or change the function to return ("", nil) on expiry (and handle re-auth flow elsewhere) to avoid surprising callers.
| binaryPath = filepath.Join(tmpDir, "stackctl") | ||
|
|
||
| cmd := exec.Command("go", "build", "-o", binaryPath, ".") | ||
| cmd.Dir = filepath.Join("..", "..") | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| panic("failed to build binary: " + string(out) + ": " + err.Error()) | ||
| } |
There was a problem hiding this comment.
The E2E test builds a binary named stackctl without adding the platform-specific executable suffix. On Windows this will produce stackctl.exe and exec.Command(binaryPath, ...) will fail. Consider appending .exe when runtime.GOOS == "windows" or using go env GOEXE to determine the suffix.
| // Client is the HTTP client for the k8s-stack-manager API. | ||
| type Client struct { | ||
| BaseURL string | ||
| Token string | ||
| APIKey string | ||
| HTTPClient *http.Client | ||
| Insecure bool | ||
| } | ||
|
|
||
| // New creates a new API client. | ||
| func New(baseURL string) *Client { | ||
| return &Client{ | ||
| BaseURL: strings.TrimRight(baseURL, "/"), | ||
| HTTPClient: &http.Client{ | ||
| Timeout: defaultTimeout, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Client has an Insecure field but it's never used to configure the underlying http.Client transport, so it currently has no effect. If you intend to support insecure TLS mode, configure HTTPClient.Transport (e.g., TLSClientConfig.InsecureSkipVerify) when this is set, and consider the security implications/documentation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### Go install | ||
|
|
||
| ```bash | ||
| go install github.com/omattsson/stackctl@latest |
There was a problem hiding this comment.
go install github.com/omattsson/stackctl@latest won’t work with the current repo layout because go.mod is under cli/. Update this command once the module layout is finalized (either move go.mod to the repo root, or use a /cli module path and reference that here).
| go install github.com/omattsson/stackctl@latest | |
| go install github.com/omattsson/stackctl/cli@latest |
| dir := filepath.Dir(path) | ||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return fmt.Errorf("creating config directory: %w", err) | ||
| } |
There was a problem hiding this comment.
os.MkdirAll does not change permissions on an existing directory. Since this directory can contain secrets (API keys, tokens), consider explicitly enforcing 0700 (e.g., Chmod) when the directory already exists.
| } | |
| } | |
| if err := os.Chmod(dir, 0700); err != nil { | |
| return fmt.Errorf("setting config directory permissions: %w", err) | |
| } |
| // Set up a context | ||
| _, _, err := runStackctl(t, dir, "config", "use-context", "test") | ||
| require.NoError(t, err) | ||
| _, _, err = runStackctl(t, dir, "config", "set", "api-url", "http://config-url") | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This test is named TestE2E_EnvVarOverride, but it doesn’t set any STACKCTL_* environment variables or assert precedence over config. Either rename it to reflect what it checks, or add env var setup/assertions to cover the intended override behavior.
| # 2. Authenticate | ||
| stackctl login | ||
|
|
||
| # 3. Browse templates and deploy | ||
| stackctl template list | ||
| stackctl template quick-deploy 1 | ||
|
|
||
| # 4. Monitor your stacks | ||
| stackctl stack list --mine | ||
| stackctl stack status 42 | ||
| stackctl stack logs 42 |
There was a problem hiding this comment.
The Quick Start references stackctl login / stackctl template ..., but those commands are not implemented in cli/cmd/ in this PR. Consider limiting this snippet to available commands (config/version/completion) or adding the missing command scaffolding before documenting it here.
| # 2. Authenticate | |
| stackctl login | |
| # 3. Browse templates and deploy | |
| stackctl template list | |
| stackctl template quick-deploy 1 | |
| # 4. Monitor your stacks | |
| stackctl stack list --mine | |
| stackctl stack status 42 | |
| stackctl stack logs 42 | |
| # 2. Check your stackctl version | |
| stackctl version | |
| # 3. (Optional) Enable shell autocompletion | |
| # For bash: | |
| stackctl completion bash > /etc/bash_completion.d/stackctl | |
| # For zsh: | |
| stackctl completion zsh > "${fpath[1]}/_stackctl" |
|
|
||
| if err := os.WriteFile(path, data, 0600); err != nil { | ||
| return fmt.Errorf("writing config file: %w", err) | ||
| } |
There was a problem hiding this comment.
os.WriteFile(path, ..., 0600) only applies 0600 on create; it won’t tighten permissions if config.yaml already exists with broader access. Consider enforcing 0600 after writing (or using an atomic temp file + Rename + Chmod) so secrets aren’t left world/group-readable.
| } | |
| } | |
| if err := os.Chmod(path, 0600); err != nil { | |
| return fmt.Errorf("setting config file permissions: %w", err) | |
| } |
| defer resp.Body.Close() | ||
|
|
||
| if v != nil { | ||
| if err := json.NewDecoder(resp.Body).Decode(v); err != nil { |
There was a problem hiding this comment.
doJSON will return an error when the response body is empty (common for 204 No Content or some PUT endpoints) because json.Decoder.Decode returns io.EOF. Consider treating io.EOF as success (no payload) when decoding into v.
| if err := json.NewDecoder(resp.Body).Decode(v); err != nil { | |
| if err := json.NewDecoder(resp.Body).Decode(v); err != nil { | |
| // Treat an empty response body as success for JSON decoding, which is common | |
| // for 204 No Content or some PUT/DELETE endpoints. | |
| if err == io.EOF && (resp.StatusCode == http.StatusNoContent || resp.ContentLength == 0) { | |
| return nil | |
| } |
| dir := filepath.Dir(path) | ||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return fmt.Errorf("creating token directory: %w", err) | ||
| } |
There was a problem hiding this comment.
saveToken creates the token directory with 0700, but os.MkdirAll won’t fix permissions if the directory already exists with broader access. Consider explicitly enforcing 0700 (e.g., Chmod) to avoid leaving tokens readable by other users on shared systems.
| } | |
| } | |
| if err := os.Chmod(dir, 0700); err != nil { | |
| return fmt.Errorf("setting token directory permissions: %w", err) | |
| } |
| } | ||
|
|
||
| // loadToken reads the JWT token for the current context. | ||
| // Returns empty string if no token exists or token is expired. |
There was a problem hiding this comment.
The comment on loadToken says it returns an empty string when the token is expired, but the implementation returns a non-nil error in that case. Update the comment to match behavior (or change the implementation), otherwise callers may handle expiry incorrectly.
| // Returns empty string if no token exists or token is expired. | |
| // Returns empty string and nil error if no token exists; returns an error if the token is expired. |
| } | ||
|
|
||
| // Initialize printer | ||
| printer = output.NewPrinter(flagOutput, flagQuiet, flagNoColor) |
There was a problem hiding this comment.
printer is initialized with Writer: os.Stdout, but some commands write via cmd.OutOrStdout() while others write via printer. To make output redirection consistent (and simplify tests using cmd.SetOut), consider setting printer.Writer = cmd.OutOrStdout() during init.
| printer = output.NewPrinter(flagOutput, flagQuiet, flagNoColor) | |
| printer = output.NewPrinter(flagOutput, flagQuiet, flagNoColor) | |
| // Ensure printer respects Cobra's configured output (e.g., for cmd.SetOut in tests) | |
| printer.Writer = cmd.OutOrStdout() |
|
|
||
| if err := os.WriteFile(path, data, 0600); err != nil { | ||
| return fmt.Errorf("writing token file: %w", err) | ||
| } |
There was a problem hiding this comment.
os.WriteFile(path, ..., 0600) won’t tighten permissions if the token file already exists with broader access. Since this file contains credentials, consider enforcing 0600 after write (or atomic write+rename + Chmod) to guarantee secrecy.
| } | |
| } | |
| if err := os.Chmod(path, 0600); err != nil { | |
| return fmt.Errorf("setting token file permissions: %w", err) | |
| } |
Fixes all 23 comments from Copilot code review: Security: - Enforce chmod 0700/0600 on config dirs and token/config files even if they already exist with broader permissions - Mask api-key in `config set` confirmation output (show only last 4 chars) - Wire --insecure flag and config setting through to HTTP client TLS - Print loadToken errors as warnings instead of silently ignoring - Print errors to stderr before exit (was silent with SilenceErrors) Correctness: - Fix go.mod module path to github.com/omattsson/stackctl/cli - Update all import paths accordingly - Remove unused Insecure field from Client struct (now handled by caller) - Handle empty response body (EOF) in doJSON for 204 No Content - Validate insecure config value with strconv.ParseBool - Fix loadToken comment to match actual error behavior - Set printer.Writer from cmd.OutOrStdout() for consistent output UX: - Add XDG_CONFIG_HOME support for config directory - Add --insecure global flag with warning - Rename misleading TestE2E_EnvVarOverride to TestE2E_ConfigGetAfterSet - Add Windows .exe suffix handling in e2e tests - Fix README: correct go install path, scope Quick Start to implemented commands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return fmt.Errorf("creating token directory: %w", err) | ||
| } | ||
| // Enforce 0700 even if directory already existed with broader permissions | ||
| if err := os.Chmod(dir, 0700); err != nil { | ||
| return fmt.Errorf("setting token directory permissions: %w", err) | ||
| } | ||
|
|
||
| data, err := json.Marshal(storedToken{ | ||
| Token: token, | ||
| ExpiresAt: expiresAt, | ||
| Username: username, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling token: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(path, data, 0600); err != nil { | ||
| return fmt.Errorf("writing token file: %w", err) | ||
| } | ||
| // Enforce 0600 even if file already existed with broader permissions | ||
| if err := os.Chmod(path, 0600); err != nil { | ||
| return fmt.Errorf("setting token file permissions: %w", err) | ||
| } |
There was a problem hiding this comment.
saveToken() enforces POSIX permissions via os.Chmod. On Windows this can fail or be a no-op, causing saveToken to error unexpectedly (and tests to fail). Consider making the chmod best-effort on Windows (or ignoring permission-setting errors there) and skipping strict permission assertions in tests when runtime.GOOS == "windows".
| configPath := filepath.Join(dir, "config.yaml") | ||
| info, err := os.Stat(configPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) |
There was a problem hiding this comment.
This e2e test asserts Unix permission bits (0600) on the config file. That will not be portable to Windows filesystems, and os.Chmod may behave differently there. If Windows support is expected, gate this assertion by runtime.GOOS != "windows" (or relax it).
| assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) | |
| if runtime.GOOS != "windows" { | |
| assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) | |
| } |
| cfg := &config.Config{ | ||
| CurrentContext: "test", | ||
| Contexts: map[string]*config.Context{"test": {APIURL: "http://test"}}, | ||
| } | ||
| require.NoError(t, cfg.SaveTo(configPath)) | ||
|
|
||
| info, err := os.Stat(configPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0600), info.Mode().Perm(), "config file should be owner-only readable/writable") | ||
| } |
There was a problem hiding this comment.
This integration test asserts the config file is 0600. On Windows, permission bits are not reliable and os.Chmod may not behave as expected. Consider skipping or relaxing this assertion on Windows (e.g., via runtime.GOOS).
| } | ||
|
|
||
| // Print outputs data in the configured format. For table format, it uses the provided | ||
| // headers and rowFn to extract rows. For JSON/YAML, it outputs the raw data. |
There was a problem hiding this comment.
The comment for Print mentions a rowFn parameter, but the function signature takes rows [][]string directly. Update the comment to match the current API to avoid confusion for future callers/maintainers.
| // headers and rowFn to extract rows. For JSON/YAML, it outputs the raw data. | |
| // headers and rows. For JSON/YAML, it outputs the raw data. |
| // PrintTable writes a table with headers and rows. | ||
| func (p *Printer) PrintTable(headers []string, rows [][]string) { | ||
| w := p.TableWriter() | ||
| fmt.Fprintln(w, strings.Join(headers, "\t")) | ||
| for _, row := range rows { | ||
| fmt.Fprintln(w, strings.Join(row, "\t")) | ||
| } | ||
| w.Flush() | ||
| } |
There was a problem hiding this comment.
PrintTable ignores the error returned by w.Flush(). If the underlying writer errors (broken pipe, disk full, etc.), callers won’t be able to react. Consider returning an error from PrintTable (and propagating it) or at least handling/logging the flush error consistently with PrintSingle.
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return fmt.Errorf("creating config directory: %w", err) | ||
| } | ||
| // Enforce 0700 even if directory already existed with broader permissions | ||
| if err := os.Chmod(dir, 0700); err != nil { | ||
| return fmt.Errorf("setting config directory permissions: %w", err) | ||
| } | ||
|
|
||
| data, err := yaml.Marshal(c) | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling config: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(path, data, 0600); err != nil { | ||
| return fmt.Errorf("writing config file: %w", err) | ||
| } | ||
| // Enforce 0600 even if file already existed with broader permissions | ||
| if err := os.Chmod(path, 0600); err != nil { | ||
| return fmt.Errorf("setting config file permissions: %w", err) | ||
| } |
There was a problem hiding this comment.
SaveTo() enforces POSIX permissions via os.Chmod. On Windows, Chmod/permission bits behave differently and can return errors, which would break config saving and tests. If Windows support is intended (e2e tests already handle stackctl.exe), consider skipping chmod on Windows (or tolerating errors) and gating permission assertions in tests by runtime.GOOS != "windows".
| // 2. Create token directory and write a token file | ||
| tokenDir := filepath.Dir(tokenPath) | ||
| require.NoError(t, os.MkdirAll(tokenDir, 0700)) | ||
| require.NoError(t, os.WriteFile(tokenPath, []byte(`{"token":"test-jwt","expires_at":"2099-01-01T00:00:00Z"}`), 0600)) | ||
|
|
||
| // 3. Verify file exists with correct permissions | ||
| info, err := os.Stat(tokenPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) | ||
|
|
||
| // 4. Verify directory permissions | ||
| dirInfo, err := os.Stat(tokenDir) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0700), dirInfo.Mode().Perm()) | ||
|
|
There was a problem hiding this comment.
These integration tests assert POSIX permission bits (0600/0700) for token files/directories. This is not portable to Windows and may fail depending on filesystem semantics. Consider skipping these permission assertions on Windows (or making them conditional) if Windows is a supported platform.
|
|
||
| ### Prerequisites | ||
|
|
||
| - Go 1.22+ |
There was a problem hiding this comment.
The README states a minimum Go version of 1.22+, but cli/go.mod currently declares go 1.26.1. Align these so contributors don’t get conflicting guidance (and so the module builds on the intended toolchain).
| - Go 1.22+ | |
| - Go 1.26+ |
| ## Configuration | ||
|
|
||
| stackctl uses named contexts to manage multiple environments. Configuration is stored in `~/.stackmanager/config.yaml`. | ||
|
|
||
| ### Contexts | ||
|
|
||
| ```bash | ||
| # Create and switch to a context | ||
| stackctl config use-context local | ||
| stackctl config set api-url http://localhost:8081 | ||
|
|
||
| # Add a production context | ||
| stackctl config use-context production | ||
| stackctl config set api-url https://stackmanager.example.com | ||
| stackctl config set api-key sk_prod_... | ||
|
|
||
| # Switch between contexts | ||
| stackctl config use-context local | ||
|
|
||
| # List all contexts | ||
| stackctl config list | ||
| ``` | ||
|
|
||
| ### Authentication | ||
|
|
||
| stackctl supports two authentication methods: | ||
|
|
||
| - **JWT token** — `stackctl login` prompts for credentials and stores the token in `~/.stackmanager/tokens/<context>.json` | ||
| - **API key** — `stackctl config set api-key sk_...` for non-interactive / CI use | ||
|
|
There was a problem hiding this comment.
The README documents the config/token location as ~/.stackmanager/..., but the code can also place config under $XDG_CONFIG_HOME/stackmanager or $STACKCTL_CONFIG_DIR. Consider updating this section to describe the default path and the XDG/env override behavior so users can predict where files are written.
| // TokenPath returns the path to the token file for a given context. | ||
| func TokenPath(contextName string) (string, error) { | ||
| dir, err := ConfigDir() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(dir, TokenDir, contextName+".json"), nil | ||
| } |
There was a problem hiding this comment.
TokenPath() directly concatenates contextName into a filename. If a user supplies a context name containing path separators (e.g., ../), this can write/read tokens outside the intended config directory. Consider validating context names (allowlist characters) or rejecting names where filepath.Base(contextName) != contextName / strings.ContainsRune(contextName, os.PathSeparator).
…gation - Make chmod calls best-effort on Windows (token.go, config.go) - Gate permission assertions on non-Windows in e2e and integration tests - Add YAML output support to version command - Return error from PrintTable and propagate to callers - Fix Print comment to match actual signature (rows not rowFn) - Document STACKCTL_CONFIG_DIR and XDG_CONFIG_HOME in config help text Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if flagOutput == "json" { | ||
| return printer.PrintJSON(map[string]string{ | ||
| "version": buildVersion, | ||
| "commit": buildCommit, | ||
| "date": buildDate, | ||
| }) | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "stackctl %s (commit: %s, built: %s)\n", buildVersion, buildCommit, buildDate) | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
The version command only special-cases JSON. If the global --output yaml flag is used, it currently falls back to the plain text format instead of YAML, which is inconsistent with the advertised supported output formats. Consider using the shared printer formatting path (JSON/YAML/table) for version output as well.
| if c.APIKey == "" { | ||
| token, err := loadToken() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: %v\n", err) |
There was a problem hiding this comment.
newClient prints token-load failures directly to os.Stderr, which bypasses Cobra’s configured stderr and will emit output even in --quiet mode (breaking pipeable output). Prefer routing warnings through Cobra’s cmd.ErrOrStderr() and/or suppressing them in quiet mode (or returning the warning to be handled by the caller).
| fmt.Fprintf(os.Stderr, "Warning: %v\n", err) | |
| if !flagQuiet { | |
| fmt.Fprintf(os.Stderr, "Warning: %v\n", err) | |
| } |
| func TestMain(m *testing.M) { | ||
| // Build the binary before running tests | ||
| tmpDir, err := os.MkdirTemp("", "stackctl-e2e-*") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| binaryName := "stackctl" | ||
| if runtime.GOOS == "windows" { | ||
| binaryName = "stackctl.exe" | ||
| } | ||
| binaryPath = filepath.Join(tmpDir, binaryName) | ||
|
|
||
| cmd := exec.Command("go", "build", "-o", binaryPath, ".") | ||
| cmd.Dir = filepath.Join("..", "..") | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| panic("failed to build binary: " + string(out) + ": " + err.Error()) | ||
| } |
There was a problem hiding this comment.
In short mode (go test -short), each test is skipped, but TestMain still builds the binary unconditionally. This adds unnecessary work (and can fail) even when the suite is intended to be skipped. Consider checking testing.Short() in TestMain and skipping the build when short mode is enabled.
| if v != nil { | ||
| if err := json.NewDecoder(resp.Body).Decode(v); err != nil { | ||
| // Treat empty body as success (common for 204 No Content) | ||
| if err == io.EOF { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("decoding response: %w", err) | ||
| } |
There was a problem hiding this comment.
doJSON treats io.EOF (empty response body) as success for any status code. This can mask server bugs for endpoints that are expected to return JSON (e.g., 200 with an empty body). Consider only ignoring io.EOF for 204 No Content (and possibly other explicitly empty responses), otherwise return a decode error.
| stackctl config set api-url http://localhost:8081 | ||
| stackctl login | ||
| stackctl stack list`, |
There was a problem hiding this comment.
The root command help text includes stackctl login and stackctl stack list, but those commands are not part of this scaffold yet (only config and version exist in cmd/). This makes stackctl --help misleading; consider removing these from the “Get started” block or clearly marking them as “coming in Phase …”.
| stackctl config set api-url http://localhost:8081 | |
| stackctl login | |
| stackctl stack list`, | |
| stackctl config set api-url http://localhost:8081`, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name := args[0] | ||
| if _, ok := cfg.Contexts[name]; !ok { | ||
| cfg.Contexts[name] = &config.Context{} | ||
| } |
There was a problem hiding this comment.
Context names are accepted verbatim from CLI args. Since the context name is later used in token file paths, allowing path separators or .. can lead to unsafe filesystem paths. Validate/sanitize name here (reject /, \\, .., and control chars) to prevent creating contexts that can’t be safely used.
| ## Output Formats | ||
|
|
||
| All commands support multiple output formats: | ||
|
|
||
| ```bash | ||
| # Table (default) — human-readable with colored status badges | ||
| stackctl stack list | ||
|
|
||
| # JSON — machine-readable, full API response | ||
| stackctl stack list -o json | ||
|
|
||
| # YAML — machine-readable | ||
| stackctl stack list -o yaml | ||
|
|
||
| # Quiet — IDs only, one per line (for piping) | ||
| stackctl stack list -q | ||
| ``` |
There was a problem hiding this comment.
README states that “All commands support multiple output formats”, but some commands (e.g. stackctl config list) currently always render a table regardless of --output. Either adjust the implementation so the global --output is honored consistently, or narrow the documentation to the commands that actually support JSON/YAML/quiet output today.
| if flagOutput == "json" { | ||
| return printer.PrintJSON(versionInfo) | ||
| } | ||
| if flagOutput == "yaml" { | ||
| return printer.PrintYAML(versionInfo) | ||
| } |
There was a problem hiding this comment.
version output selection checks flagOutput == "json"/"yaml" without normalizing case, while NewPrinter does normalize (strings.ToLower). As a result, -o JSON will initialize a JSON printer but still take the table output path here. Prefer switching on printer.Format or using strings.ToLower(flagOutput) for these comparisons.
| // TokenPath returns the path to the token file for a given context. | ||
| func TokenPath(contextName string) (string, error) { | ||
| dir, err := ConfigDir() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(dir, TokenDir, contextName+".json"), nil | ||
| } |
There was a problem hiding this comment.
TokenPath builds a filesystem path directly from contextName without validation. A context name containing path separators or .. can escape the tokens/ directory (path traversal) and potentially overwrite arbitrary files under the config dir. Reject context names containing /, \, or .. (or restrict to a safe charset) before joining paths.
| package config | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strconv" | ||
|
|
||
| "gopkg.in/yaml.v3" | ||
| ) |
There was a problem hiding this comment.
PR description mentions a Viper-based config implementation, but this package reads/writes YAML directly and there’s no Viper usage in the CLI. Either update the PR description to match the implementation or switch the config loading/writing to Viper as described.
| c.HTTPClient.Transport = &http.Transport{ | ||
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // user-requested | ||
| } |
There was a problem hiding this comment.
When --insecure is set, this replaces the client's transport with a new http.Transport that only sets TLSClientConfig. That drops important defaults (proxy from env, dial/handshake timeouts, keep-alives), which can cause hangs or unexpected behavior. Prefer cloning http.DefaultTransport (or the existing transport) and only modifying TLSClientConfig.
| c.HTTPClient.Transport = &http.Transport{ | |
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // user-requested | |
| } | |
| // Preserve existing transport settings (proxies, timeouts, keep-alives) | |
| // by cloning the current/default *http.Transport and only adjusting TLS. | |
| baseTransport := http.DefaultTransport | |
| if c.HTTPClient != nil && c.HTTPClient.Transport != nil { | |
| baseTransport = c.HTTPClient.Transport | |
| } | |
| if t, ok := baseTransport.(*http.Transport); ok { | |
| clone := t.Clone() | |
| if clone.TLSClientConfig == nil { | |
| clone.TLSClientConfig = &tls.Config{} | |
| } | |
| clone.TLSClientConfig.InsecureSkipVerify = true //nolint:gosec // user-requested | |
| c.HTTPClient.Transport = clone | |
| } else { | |
| // Fallback for non-*http.Transport implementations. | |
| c.HTTPClient.Transport = &http.Transport{ | |
| TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // user-requested | |
| } | |
| } |
| headers := []string{"", "CONTEXT", "API URL", "API KEY", "INSECURE"} | ||
| var rows [][]string | ||
| for _, name := range names { | ||
| ctx := cfg.Contexts[name] | ||
| marker := " " | ||
| if name == cfg.CurrentContext { | ||
| marker = "*" | ||
| } | ||
| apiKey := "" | ||
| if ctx.APIKey != "" { | ||
| // Mask API key, show only last 4 chars | ||
| if len(ctx.APIKey) > 4 { | ||
| apiKey = "***" + ctx.APIKey[len(ctx.APIKey)-4:] | ||
| } else { | ||
| apiKey = "***" | ||
| } | ||
| } | ||
| insecure := "" | ||
| if ctx.Insecure { | ||
| insecure = "true" | ||
| } | ||
| rows = append(rows, []string{marker, name, ctx.APIURL, apiKey, insecure}) | ||
| } | ||
| return printer.PrintTable(headers, rows) | ||
| }, |
There was a problem hiding this comment.
config list always prints a table via PrintTable, so --output json|yaml has no effect even though --output is a global flag. Consider routing this through printer.Print(...) (providing both the structured data and table headers/rows) so config listing respects the chosen output format.
- Validate context names to prevent path traversal (reject /, \, ..) - Switch version output on printer.Format instead of raw flagOutput string - Clone http.DefaultTransport for insecure mode to preserve defaults - Suppress token warning in --quiet mode - Only treat io.EOF as success for 204 No Content, not all status codes - Make config list honor --output json/yaml via printer.Print - Skip binary build in e2e TestMain when running in short mode - Remove unimplemented commands from root help text - Narrow README "all commands" output format claim Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
* feat(plugin): external command discovery (git/kubectl/gh style) 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> * 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) <noreply@anthropic.com> * feat(cli)!: remove `stack refresh-db` — migrated to external plugin 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. * docs(plugins): add EXTENDING.md guide for stackctl plugin authors 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. * chore(plugin): address code-review feedback - 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. * chore(plugin): address Copilot review feedback on PR #35 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. * 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) <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Olof Mattsson <olof.mattsson@klaravik.se> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
--output,--quiet,--no-color,--api-url,--api-key)config set/get/list/use-context/current-context/delete-contextcommands0600permissions and expiry detectionversioncommand with build-time ldflags supportTest Coverage (93 tests, 78.3% overall)
pkg/clientpkg/outputpkg/configcmd/test/integration/test/e2e/Test plan
go test ./... -count=1— all 93 tests passgo vet ./...— cleango build -o bin/stackctl .— binary builds and runsconfig listoutputCloses #1
🤖 Generated with Claude Code