Skip to content

refactor: adopt Ghost's config and client loading patterns - #183

Merged
nathanjcochran merged 4 commits into
mainfrom
nathan/improve-config
Aug 5, 2026
Merged

refactor: adopt Ghost's config and client loading patterns#183
nathanjcochran merged 4 commits into
mainfrom
nathan/improve-config

Conversation

@nathanjcochran

Copy link
Copy Markdown
Member

Reworks how Tiger CLI loads its configuration and API client, adopting several patterns from ghost. The public interface is unchanged — same commands, flags, config keys, and environment variables — this is all internals.

The starting point: config lived in a process-wide viper instance that code read from directly, and both the config and the API client were loaded several times per invocation. That made the precedence rules (flag > env > file > default) hard to reason about, let one command's flag bindings leak into another's, and produced at least one real bug.

Config loading

  • No global viper. config.Load(flags) builds its own viper instance per call and unmarshals it into a Config; nothing reads a process-wide instance anymore. Config is now just a value that gets passed around.
  • Fixed: tiger config set used to override env vars and flags. It wrote the config file and called viper.Set on the global instance, which is a hard override — so for the rest of that process, the value it wrote beat any TIGER_* env var or CLI flag. Set/Unset/Reset now write the file and reload the struct through normal precedence.
  • One flag-binding table. The flags that override config values are declared in a single map and applied per load against the flag set of the command being run. This replaces a PreRunE hook that each command had to remember to add, and means a command-local flag like --output binds only for the commands that define it, so commands can no longer stomp each other's bindings.
  • Simpler write path. A ~200-line switch that validated values and mutated both the struct and viper collapsed into a small validateValue, and tiger config show now goes through one helper instead of assembling a viper by hand.
  • Credentials are methods on the config, keyed off its config directory, rather than locating that directory through the global viper — so --config-dir is now honored consistently for credential storage.

Loading the config and client once (the App)

  • Loaded once per invocation. A new common.App holds the config and API client; a wrapper around every command's RunE loads them once and shares them through accessors. Commands previously caused 3–4 config loads and two credential/keyring reads per run.
  • Replaced the common.Config wrapper, which embedded *config.Config alongside the client and project ID — two different types named "Config", plus a lot of cfg.Config unwrapping at call sites.
  • One lifecycle instead of two hooks. PersistentPreRunE/PersistentPostRunE are gone. Loading, logging setup, color, the update check, and analytics now happen in one wrapper, in a readable order.
  • --help and completions no longer load anything. The commands cobra adds after wrapping (help, completion, __complete) skip the lifecycle entirely, so help output and tab completion no longer read the config file or touch the system keyring. The two completions that genuinely need the API opt in explicitly.
  • The update check still overlaps the command's own work, but is now one function (start the fetch, print on the way out) instead of being split across two lifecycle hooks, and it correctly stays quiet if the command turned version_check off.
  • MCP loads once per request. The server holds the App and reloads it in the analytics middleware; tool handlers read that state rather than each loading their own config and client. Config changes and logins/logouts still take effect on the next tool call, as before.
  • TIGER_EXPERIMENTAL is read once into the App, instead of separately by the CLI and the MCP server.

Tests

  • The API client is now referred to by its interface, so tests can inject a mock, and the App has a client-factory seam for exactly that. A follow-up PR will move the test machinery over to it.
  • Fixed test isolation: a few tests were reading the real ~/.config/tiger/config.yaml of whoever ran them. Tests now always get an isolated config directory.

Behavior worth knowing about while reviewing

  • config reset/unset no longer mask env vars for the remainder of the process — that was a side effect of the override bug above; the effective value now re-resolves normally.
  • tiger config show no longer creates the config directory as a side effect.
  • An undocumented new_password config-file key is no longer read. The --new-password flag and TIGER_NEW_PASSWORD work as before.
  • tiger auth logout's analytics event is now attributed to the user who logged out, instead of going out unauthenticated.

@nathanjcochran nathanjcochran self-assigned this Aug 4, 2026
@nathanjcochran
nathanjcochran marked this pull request as ready for review August 4, 2026 21:35

@Askir Askir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall 🚀

Comment thread internal/cmd/root.go Outdated
Comment on lines 35 to 41
var configDir string
var debug bool
var serviceID string
var analytics bool
var analyticsEnabled bool
var passwordStorage string
var skipUpdateCheck bool
var colorFlag bool

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these variables are all completely redundant. At least Claude tells me we could just use .StringP instead of .StringVarP and then these variables disappear. I mention this because there is always the theoretical mistake one can make by reading from the variable directly instead of from the config struct, which would bypass the viper load order.
Idk if that's also worth cleaning up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. Addressed here: 34049b6.

Claude also noticed that the same is true for the various output flags defined for various commands - they don't need a local variable, because the intention is for the output format to always be read from the config, and the flag is just one way of setting the config value. That also uncovered a bug where tiger version was reading the local flag variable directly, and therefore not respecting the value in the config or env var. So that was fixed as well.

@nathanjcochran
nathanjcochran force-pushed the nathan/improve-config branch from 3c8d98e to c43fc5a Compare August 5, 2026 21:15
@nathanjcochran
nathanjcochran merged commit b046c30 into main Aug 5, 2026
2 checks passed
@nathanjcochran
nathanjcochran deleted the nathan/improve-config branch August 5, 2026 22:03
gonzaloserrano added a commit that referenced this pull request Aug 10, 2026
main gained *config.Config on the connection helpers and moved command
wiring to App.GetAll (#183), which lands as a silent break here: the
merge is textually clean but the call sites stop compiling.

Thread Config through ConnectableWaitArgs, take the client as
api.ClientWithResponsesInterface to match WaitForServiceArgs, and read
client/projectID from App.GetAll at the three call sites.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants