fix(cli): deployment-scoped commands resolve --repo/--pg-connection from config (closes #12)#13
Merged
Conversation
#12) `pg_hardstorage backup <deployment>` demanded --pg-connection and --repo even when the named deployment was fully configured in pg_hardstorage.yaml — it never consulted the deployment catalogue. Now, when those flags are omitted, backup loads the named deployment from config and fills pg-connection/repo from it (explicit flags still win); the required-flag check validates the resolved values, so a configured deployment "just works" and an unknown one still errors with usage.missing_flag. Adds a regression test, and isolates TestBackup_Requires{PGConnection,Repo} from any ambient ~/.config config now that backup reads it. Closes #12.
Extract a shared, tested helper (deployment_defaults.go) and apply it to the backup/restore lifecycle so a configured deployment no longer demands flags it already declares: - backup → resolves --pg-connection and --repo from the deployment - restore → resolves --repo (--target stays CLI-only; not in config) - verify / verify --full → resolve --repo Tests: - deployment_defaults_test.go: table-driven unit test for the resolver (fills both, flag-wins precedence, partial, unknown/empty deployment, nil deps). - backup/restore/verify CLI regression tests for "configured deployment, no flags" and "no config, flag still required". - Isolated pre-existing TestBackup_Requires* / TestRestore_Requires* from any ambient ~/.config config now that these commands read it. NOTE — broader systemic gap (NOT yet fixed): --repo is MarkFlagRequired on ~40 other deployment-scoped commands (list, show, hold, rotate, status, recovery, repair, wal preflight/stream/list/audit/prune, partial, kms verify/shred, ...) and pg-connection on the wal commands; none consult the deployment config. Same bug class — tracked for a follow-up sweep.
…ommands (#12) The reported bug was systemic: --repo (and --pg-connection on the wal commands) is MarkFlagRequired on ~40 deployment-scoped commands, none of which consulted the deployment catalogue — so a configured deployment still demanded the flags everywhere (list, show, status, hold, rotate, recovery, repair, wal preflight/stream/list/audit/prune/gaps, partial, kms verify/shred, …). Fix at one point: a root PersistentPreRunE resolver (resolveDeploymentDefaultsPreRun) runs before Cobra validates required flags and fills an unset --repo / --pg-connection from the deployment named as the command's first positional arg. It is conservative — acts only when the flag exists, is unset, and the arg names a known deployment — so non-deployment commands are unaffected and a genuine omission still errors. Combined with the earlier requireFlags-path fix for backup/restore/verify, every deployment-scoped command now honours pg_hardstorage.yaml. Tests proving it: - deployment_flag_resolution_test.go: a 14-command matrix (configured deployment, no flags → no usage.missing_flag) + a negative control (no config → still usage.missing_flag). - deployment_defaults_test.go: table-driven resolver unit test. - per-command regression tests for backup/restore/verify. - main_test.go: package TestMain pins a clean HOME so the suite is deterministic regardless of the host's ~/.config/pg_hardstorage (this ambient-config leak is what surfaced the breakage). Full internal/cli package, go build ./..., and go vet pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (#12)
pg_hardstorage backup mytesterroredbackup: --pg-connection, --repo are requiredeven thoughmytestwas fully configured inpg_hardstorage.yaml. The command never consulted the deployment catalogue.It turned out to be systemic, not unique to
backup:--repo(and--pg-connectionon thewalcommands) is enforced on ~40 deployment-scoped commands via two mechanisms —requireFlags()(backup/restore/verify) and cobra'sMarkFlagRequired()(list, show, status, hold, rotate, recovery, repair,wal preflight/stream/list/audit/prune/gaps, partial,kms verify/shred, …) — and none loaded config.Fix
deploymentDefaults()for therequireFlags()path (backup/restore/verify/verify --full).PersistentPreRunEresolver (resolveDeploymentDefaultsPreRun) that runs before cobra validates required flags and fills an unset--repo/--pg-connectionfrom the deployment named as the command's first positional arg. One point, covers everyMarkFlagRequiredcommand.args[0]names a known deployment. Explicit flags always win; non-deployment commands and genuinely missing flags still error exactly as before. Repo-only commands (fleet/capacity/audit/…) are correctly untouched.Tests (proof)
TestDeploymentFlagResolution_Matrix— 14 deployment-scoped commands: configured deployment + no flags → nousage.missing_flag.TestDeploymentFlagResolution_NoConfigStillRequires— negative control: no config → stillusage.missing_flag.TestResolveDeploymentDefaults— table-driven resolver unit test (precedence, partial, unknown/empty/nil).main_test.goTestMainpins a cleanHOMEso the suite is deterministic regardless of the host's~/.config/pg_hardstorage(an ambient-config leak this work surfaced).Full
internal/clipackage,go build ./..., andgo vetpass locally.Scope notes
wal streamshares the fix via the root resolver but is intentionally excluded from the matrix (long-running; would block a unit test).logicaluses a separate config store anddb_extensiontakes no<deployment>— both correctly out of scope.Closes #12.