From ceb2790c815b95d4e8fdd186476d36a076a9a7e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hans-J=C3=BCrgen=20Sch=C3=B6nig?= Date: Wed, 24 Jun 2026 21:16:05 +0200 Subject: [PATCH 1/4] fix(backup): resolve --pg-connection/--repo from the deployment config (#12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pg_hardstorage backup ` 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. --- internal/cli/backup.go | 39 ++++++++++++++++++++++++++++++------- internal/cli/backup_test.go | 34 ++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/internal/cli/backup.go b/internal/cli/backup.go index e8945c1..4281203 100644 --- a/internal/cli/backup.go +++ b/internal/cli/backup.go @@ -16,6 +16,7 @@ import ( "github.com/cybertec-postgresql/pg_hardstorage/internal/backup/runner" "github.com/cybertec-postgresql/pg_hardstorage/internal/backup/tarsink" "github.com/cybertec-postgresql/pg_hardstorage/internal/capacity" + "github.com/cybertec-postgresql/pg_hardstorage/internal/config" "github.com/cybertec-postgresql/pg_hardstorage/internal/kms" "github.com/cybertec-postgresql/pg_hardstorage/internal/output" "github.com/cybertec-postgresql/pg_hardstorage/internal/paths" @@ -170,13 +171,37 @@ func runBackup(cmd *cobra.Command, opts runOptions) error { return runBackupControlPlane(cmd, opts) } - // Required-field checks. We surface usage errors with the right - // exit-code mapping so the CLI contract is consistent. - // Conditional requirement: needed only in local mode — a control-plane - // dispatch returned above. requireFlags is MarkFlagRequired's runtime - // sibling for exactly this case. - if err := requireFlags(cmd, "pg-connection", "repo"); err != nil { - return err + // `backup ` resolves --pg-connection / --repo from the + // named deployment in pg_hardstorage.yaml when the operator didn't + // pass them on the command line; explicit flags still win. Without + // this, a configured deployment would wrongly demand the flags (#12). + if opts.deployment != "" && (opts.pgConn == "" || opts.repoURL == "") { + if p, perr := paths.Resolve(paths.DefaultOptions()); perr == nil { + if loaded, lerr := config.Load(p); lerr == nil { + if dep, ok := loaded.Config.Deployments[opts.deployment]; ok { + if opts.pgConn == "" { + opts.pgConn = dep.PGConnection + } + if opts.repoURL == "" { + opts.repoURL = dep.Repo + } + } + } + } + } + + // Required-field checks (local mode only — a control-plane dispatch + // returned above). Validate the resolved values, not the raw flags: + // they may have been filled from the deployment config just above. + var missing []string + if opts.pgConn == "" { + missing = append(missing, "--pg-connection") + } + if opts.repoURL == "" { + missing = append(missing, "--repo") + } + if len(missing) > 0 { + return missingFlagErr(cmd, missing...) } // Resolve the keyring path via the same precedence chain doctor diff --git a/internal/cli/backup_test.go b/internal/cli/backup_test.go index 6273bfd..384fe41 100644 --- a/internal/cli/backup_test.go +++ b/internal/cli/backup_test.go @@ -32,6 +32,12 @@ func TestBackup_RequiresDeployment(t *testing.T) { } func TestBackup_RequiresPGConnection(t *testing.T) { + // Isolate from any ambient config so `backup db1` has no deployment + // to resolve --pg-connection from (it now reads the config — #12). + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") stdout, errb, exit := runBackup(t, "db1", "--repo", "file:///tmp/x", "-o", "json") if exit != int(output.ExitMisuse) { t.Errorf("missing --pg-connection should map to ExitMisuse(%d); got %d", output.ExitMisuse, exit) @@ -55,6 +61,12 @@ func TestBackup_RequiresPGConnection(t *testing.T) { } func TestBackup_RequiresRepo(t *testing.T) { + // Isolate from any ambient config so `backup db1` has no deployment + // to resolve --repo from (it now reads the config — #12). + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") _, errb, exit := runBackup(t, "db1", "--pg-connection", "postgres://x@y/z", "-o", "json") if exit != int(output.ExitMisuse) { t.Errorf("missing --repo should map to ExitMisuse; got %d", exit) @@ -71,6 +83,28 @@ func TestBackup_RequiresRepo(t *testing.T) { } } +// Regression for #12: `backup ` must resolve --pg-connection +// and --repo from the named deployment in the config when the operator +// omits them on the command line, instead of demanding the flags. +func TestBackup_ResolvesFlagsFromDeployment(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + repoDir := t.TempDir() + t.Setenv("PG_HARDSTORAGE_CONFIG", + "deployments:\n"+ + " mytest:\n"+ + " pg_connection: postgresql://postgres@127.0.0.1:5432/postgres\n"+ + " repo: file://"+repoDir+"\n") + + // The backup still fails (empty repo / unreachable PG), but it must + // get PAST the required-flag gate — i.e. NOT a usage.missing_flag. + _, errb, _ := runBackup(t, "mytest", "-o", "json") + if strings.Contains(errb, "usage.missing_flag") { + t.Fatalf("backup of a configured deployment must not demand --pg-connection/--repo (issue #12); stderr:\n%s", errb) + } +} + func TestBackup_BadDSN_StructuredError(t *testing.T) { // A malformed DSN is detected by pg.Connect before any I/O — we // don't need a real PG to exercise this path. The keystore is From 33d7b214156553d781cbed1e7639efa432530ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hans-J=C3=BCrgen=20Sch=C3=B6nig?= Date: Wed, 24 Jun 2026 21:25:57 +0200 Subject: [PATCH 2/4] fix(cli): resolve repo/pg-connection from deployment config; tests (#12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/cli/backup.go | 19 +------- internal/cli/deployment_defaults.go | 57 ++++++++++++++++++++++++ internal/cli/deployment_defaults_test.go | 50 +++++++++++++++++++++ internal/cli/restore.go | 16 +++++-- internal/cli/restore_test.go | 28 ++++++++++++ internal/cli/verify.go | 6 ++- internal/cli/verify_full.go | 6 ++- internal/cli/verify_resolve_test.go | 51 +++++++++++++++++++++ 8 files changed, 209 insertions(+), 24 deletions(-) create mode 100644 internal/cli/deployment_defaults.go create mode 100644 internal/cli/deployment_defaults_test.go create mode 100644 internal/cli/verify_resolve_test.go diff --git a/internal/cli/backup.go b/internal/cli/backup.go index 4281203..fd4d50b 100644 --- a/internal/cli/backup.go +++ b/internal/cli/backup.go @@ -16,7 +16,6 @@ import ( "github.com/cybertec-postgresql/pg_hardstorage/internal/backup/runner" "github.com/cybertec-postgresql/pg_hardstorage/internal/backup/tarsink" "github.com/cybertec-postgresql/pg_hardstorage/internal/capacity" - "github.com/cybertec-postgresql/pg_hardstorage/internal/config" "github.com/cybertec-postgresql/pg_hardstorage/internal/kms" "github.com/cybertec-postgresql/pg_hardstorage/internal/output" "github.com/cybertec-postgresql/pg_hardstorage/internal/paths" @@ -173,22 +172,8 @@ func runBackup(cmd *cobra.Command, opts runOptions) error { // `backup ` resolves --pg-connection / --repo from the // named deployment in pg_hardstorage.yaml when the operator didn't - // pass them on the command line; explicit flags still win. Without - // this, a configured deployment would wrongly demand the flags (#12). - if opts.deployment != "" && (opts.pgConn == "" || opts.repoURL == "") { - if p, perr := paths.Resolve(paths.DefaultOptions()); perr == nil { - if loaded, lerr := config.Load(p); lerr == nil { - if dep, ok := loaded.Config.Deployments[opts.deployment]; ok { - if opts.pgConn == "" { - opts.pgConn = dep.PGConnection - } - if opts.repoURL == "" { - opts.repoURL = dep.Repo - } - } - } - } - } + // pass them on the command line; explicit flags still win (#12). + opts.pgConn, opts.repoURL = deploymentDefaults(opts.deployment, opts.pgConn, opts.repoURL) // Required-field checks (local mode only — a control-plane dispatch // returned above). Validate the resolved values, not the raw flags: diff --git a/internal/cli/deployment_defaults.go b/internal/cli/deployment_defaults.go new file mode 100644 index 0000000..145a0e6 --- /dev/null +++ b/internal/cli/deployment_defaults.go @@ -0,0 +1,57 @@ +// deployment_defaults.go — shared resolution of connection/repo defaults +// from the deployment catalogue in pg_hardstorage.yaml. +// +// Commands that take a argument (backup, restore, verify, …) +// should let the operator omit --pg-connection / --repo when the named +// deployment already declares them in config. This is the single place +// that resolves those defaults so every command behaves identically and +// no command silently demands flags a configured deployment already has +// (issue #12). +package cli + +import ( + "github.com/cybertec-postgresql/pg_hardstorage/internal/config" + "github.com/cybertec-postgresql/pg_hardstorage/internal/paths" +) + +// resolveDeploymentDefaults fills empty pgConn / repoURL from the named +// deployment in deps. Explicit (non-empty) values always win, so a flag +// passed on the command line overrides the configured value. A deployment +// that isn't in deps (or empty fields) leaves the inputs untouched. +func resolveDeploymentDefaults(deployment, pgConn, repoURL string, deps map[string]config.DeploymentConfig) (string, string) { + if deployment == "" { + return pgConn, repoURL + } + dep, ok := deps[deployment] + if !ok { + return pgConn, repoURL + } + if pgConn == "" { + pgConn = dep.PGConnection + } + if repoURL == "" { + repoURL = dep.Repo + } + return pgConn, repoURL +} + +// deploymentDefaults loads the on-disk config and applies +// resolveDeploymentDefaults. It short-circuits when there is nothing to +// resolve. Path/config-load errors are deliberately non-fatal: the +// caller's required-flag check still fires if nothing was resolved, so a +// missing config degrades to the same "flag is required" error as before +// rather than a confusing load failure. +func deploymentDefaults(deployment, pgConn, repoURL string) (string, string) { + if deployment == "" || (pgConn != "" && repoURL != "") { + return pgConn, repoURL + } + p, err := paths.Resolve(paths.DefaultOptions()) + if err != nil { + return pgConn, repoURL + } + loaded, err := config.Load(p) + if err != nil { + return pgConn, repoURL + } + return resolveDeploymentDefaults(deployment, pgConn, repoURL, loaded.Config.Deployments) +} diff --git a/internal/cli/deployment_defaults_test.go b/internal/cli/deployment_defaults_test.go new file mode 100644 index 0000000..0b5cffe --- /dev/null +++ b/internal/cli/deployment_defaults_test.go @@ -0,0 +1,50 @@ +package cli + +import ( + "testing" + + "github.com/cybertec-postgresql/pg_hardstorage/internal/config" +) + +func TestResolveDeploymentDefaults(t *testing.T) { + deps := map[string]config.DeploymentConfig{ + "mytest": {PGConnection: "postgres://cfg@host/db", Repo: "file:///cfg/repo"}, + "norepo": {PGConnection: "postgres://cfg@host/db"}, + "empty": {}, + } + + cases := []struct { + name string + deployment string + pgIn, repoIn string + wantPG, wantRepo string + }{ + {"fills both from config", "mytest", "", "", "postgres://cfg@host/db", "file:///cfg/repo"}, + {"explicit pg wins, repo filled", "mytest", "postgres://flag@h/d", "", "postgres://flag@h/d", "file:///cfg/repo"}, + {"explicit repo wins, pg filled", "mytest", "", "file:///flag/repo", "postgres://cfg@host/db", "file:///flag/repo"}, + {"both explicit unchanged", "mytest", "postgres://flag@h/d", "file:///flag/repo", "postgres://flag@h/d", "file:///flag/repo"}, + {"config missing repo field", "norepo", "", "", "postgres://cfg@host/db", ""}, + {"config empty fields", "empty", "", "", "", ""}, + {"unknown deployment unchanged", "ghost", "", "", "", ""}, + {"empty deployment name unchanged", "", "", "file:///flag/repo", "", "file:///flag/repo"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gotPG, gotRepo := resolveDeploymentDefaults(tc.deployment, tc.pgIn, tc.repoIn, deps) + if gotPG != tc.wantPG { + t.Errorf("pgConn = %q, want %q", gotPG, tc.wantPG) + } + if gotRepo != tc.wantRepo { + t.Errorf("repoURL = %q, want %q", gotRepo, tc.wantRepo) + } + }) + } +} + +func TestResolveDeploymentDefaults_NilDeps(t *testing.T) { + pg, repo := resolveDeploymentDefaults("mytest", "", "", nil) + if pg != "" || repo != "" { + t.Errorf("nil deps must leave inputs untouched; got pg=%q repo=%q", pg, repo) + } +} diff --git a/internal/cli/restore.go b/internal/cli/restore.go index eca0a74..305e3a8 100644 --- a/internal/cli/restore.go +++ b/internal/cli/restore.go @@ -208,9 +208,19 @@ func runRestore(cmd *cobra.Command, opts restoreOpts) error { return runRestoreControlPlane(cmd, opts) } - // Conditional: only in local mode (control-plane dispatch returned above). - if err := requireFlags(cmd, "repo", "target"); err != nil { - return err + // Resolve --repo from the named deployment in config when omitted + // (explicit flag wins); --target is restore-specific and not stored + // in config. Local mode only — control-plane dispatch returned above (#12). + _, opts.repoURL = deploymentDefaults(opts.deployment, "", opts.repoURL) + var missing []string + if opts.repoURL == "" { + missing = append(missing, "--repo") + } + if f := cmd.Flags().Lookup("target"); f == nil || f.Value.String() == "" { + missing = append(missing, "--target") + } + if len(missing) > 0 { + return missingFlagErr(cmd, missing...) } verifyMode, err := restore.ParseVerifyMode(opts.verifyMode) diff --git a/internal/cli/restore_test.go b/internal/cli/restore_test.go index 42247c4..e47d530 100644 --- a/internal/cli/restore_test.go +++ b/internal/cli/restore_test.go @@ -36,6 +36,12 @@ func TestRestore_RequiresPositionalArgs(t *testing.T) { } func TestRestore_RequiresRepo(t *testing.T) { + // Isolate from ambient config so `restore db1` has no deployment to + // resolve --repo from (it now reads the config — #12). + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") stdout, errb, exit := runRestore(t, "db1", "abc123", "--target", "/tmp/x", "-o", "json") if exit != int(output.ExitMisuse) { t.Errorf("missing --repo should map to ExitMisuse(%d); got %d", output.ExitMisuse, exit) @@ -59,6 +65,10 @@ func TestRestore_RequiresRepo(t *testing.T) { } func TestRestore_RequiresTarget(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") _, errb, exit := runRestore(t, "db1", "abc123", "--repo", "file:///tmp/x", "-o", "json") if exit != int(output.ExitMisuse) { t.Errorf("missing --target should map to ExitMisuse; got %d", exit) @@ -72,6 +82,24 @@ func TestRestore_RequiresTarget(t *testing.T) { } } +// Regression for #12: `restore --target ...` must +// resolve --repo from the named deployment in config when omitted. +func TestRestore_ResolvesRepoFromDeployment(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + repoDir := t.TempDir() + t.Setenv("PG_HARDSTORAGE_CONFIG", + "deployments:\n mytest:\n repo: file://"+repoDir+"\n") + + // No --repo: it must be taken from the deployment. The restore still + // fails (empty dir isn't a repo), but NOT with usage.missing_flag. + _, errb, _ := runRestore(t, "mytest", "latest", "--target", t.TempDir()+"/r", "-o", "json") + if strings.Contains(errb, "usage.missing_flag") { + t.Fatalf("restore of a configured deployment must not demand --repo (issue #12); stderr:\n%s", errb) + } +} + func TestRestore_RepoNotARepo_NotFound(t *testing.T) { t.Setenv("HOME", t.TempDir()) t.Setenv("PG_HARDSTORAGE_ROOT", "") diff --git a/internal/cli/verify.go b/internal/cli/verify.go index 17907e0..3497bc3 100644 --- a/internal/cli/verify.go +++ b/internal/cli/verify.go @@ -126,8 +126,10 @@ Backup ID: func runVerify(cmd *cobra.Command, deployment, backupID, repoURL string, sample int, existenceOnly bool) error { d := DispatcherFrom(cmd) - if err := requireFlags(cmd, "repo"); err != nil { - return err + // Resolve --repo from the named deployment in config when omitted (#12). + _, repoURL = deploymentDefaults(deployment, "", repoURL) + if repoURL == "" { + return missingFlagErr(cmd, "--repo") } if sample < 0 { return output.NewError("usage.bad_flag", diff --git a/internal/cli/verify_full.go b/internal/cli/verify_full.go index 18f7a4b..f058d4b 100644 --- a/internal/cli/verify_full.go +++ b/internal/cli/verify_full.go @@ -52,8 +52,10 @@ import ( // differ from the source PG. func runVerifyFull(cmd *cobra.Command, deployment, backupID, repoURL, pgMajorOverride string) error { d := DispatcherFrom(cmd) - if err := requireFlags(cmd, "repo"); err != nil { - return err + // Resolve --repo from the named deployment in config when omitted (#12). + _, repoURL = deploymentDefaults(deployment, "", repoURL) + if repoURL == "" { + return missingFlagErr(cmd, "--repo") } verifier, err := loadVerifier() diff --git a/internal/cli/verify_resolve_test.go b/internal/cli/verify_resolve_test.go new file mode 100644 index 0000000..5d613f0 --- /dev/null +++ b/internal/cli/verify_resolve_test.go @@ -0,0 +1,51 @@ +package cli_test + +import ( + "bytes" + "strings" + "testing" + + "github.com/cybertec-postgresql/pg_hardstorage/internal/cli" +) + +func invokeVerify(t *testing.T, args ...string) (stderr string, exit int) { + t.Helper() + root := cli.NewRoot() + var out, errb bytes.Buffer + root.SetOut(&out) + root.SetErr(&errb) + root.SetArgs(append([]string{"verify"}, args...)) + exit = cli.Run(root) + return errb.String(), exit +} + +// Regression for #12: `verify ` must resolve --repo from the +// named deployment in config when omitted, not demand the flag. +func TestVerify_ResolvesRepoFromDeployment(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + repoDir := t.TempDir() + t.Setenv("PG_HARDSTORAGE_CONFIG", + "deployments:\n mytest:\n repo: file://"+repoDir+"\n") + + // No --repo: it must come from the deployment. Verify still fails + // (empty dir isn't a repo), but NOT with usage.missing_flag. + errb, _ := invokeVerify(t, "mytest", "latest", "-o", "json") + if strings.Contains(errb, "usage.missing_flag") { + t.Fatalf("verify of a configured deployment must not demand --repo (issue #12); stderr:\n%s", errb) + } +} + +// Without a configured deployment or --repo, verify still errors with +// usage.missing_flag (the resolution must not paper over a real omission). +func TestVerify_RequiresRepo_NoConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") + errb, _ := invokeVerify(t, "db1", "latest", "-o", "json") + if !strings.Contains(errb, "usage.missing_flag") { + t.Fatalf("verify without --repo or config should be usage.missing_flag; stderr:\n%s", errb) + } +} From 5183b92557d0f904497dabbb92556c69a0dd8209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hans-J=C3=BCrgen=20Sch=C3=B6nig?= Date: Wed, 24 Jun 2026 21:36:22 +0200 Subject: [PATCH 3/4] fix(cli): resolve repo/pg-connection from config for ALL deployment commands (#12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/cli/deployment_defaults.go | 49 +++++++++++ .../cli/deployment_flag_resolution_test.go | 84 +++++++++++++++++++ internal/cli/main_test.go | 30 +++++++ internal/cli/root.go | 2 +- 4 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 internal/cli/deployment_flag_resolution_test.go create mode 100644 internal/cli/main_test.go diff --git a/internal/cli/deployment_defaults.go b/internal/cli/deployment_defaults.go index 145a0e6..46e73b4 100644 --- a/internal/cli/deployment_defaults.go +++ b/internal/cli/deployment_defaults.go @@ -10,6 +10,8 @@ package cli import ( + "github.com/spf13/cobra" + "github.com/cybertec-postgresql/pg_hardstorage/internal/config" "github.com/cybertec-postgresql/pg_hardstorage/internal/paths" ) @@ -55,3 +57,50 @@ func deploymentDefaults(deployment, pgConn, repoURL string) (string, string) { } return resolveDeploymentDefaults(deployment, pgConn, repoURL, loaded.Config.Deployments) } + +// resolveDeploymentDefaultsPreRun is the root PersistentPreRunE handler +// (added to the chain in NewRoot) that fills an unset --repo / +// --pg-connection from the deployment named as the command's first +// positional argument, *before* Cobra validates required flags. This +// makes every deployment-scoped command honour the deployment catalogue +// in pg_hardstorage.yaml (#12) instead of demanding flags a configured +// deployment already declares. +// +// It is deliberately conservative: it acts only when the command has the +// flag, the flag is unset on the command line, and the first argument +// names a known deployment. A command whose first argument is not a +// deployment sees a lookup miss and is left untouched, so it still errors +// exactly as before. Path/config-load failures are non-fatal — Cobra's +// required-flag check then fires just as it did previously. +func resolveDeploymentDefaultsPreRun(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return nil + } + fl := cmd.Flags() + repoF := fl.Lookup("repo") + pgF := fl.Lookup("pg-connection") + repoNeed := repoF != nil && !repoF.Changed + pgNeed := pgF != nil && !pgF.Changed + if !repoNeed && !pgNeed { + return nil + } + p, err := paths.Resolve(paths.DefaultOptions()) + if err != nil { + return nil + } + loaded, err := config.Load(p) + if err != nil { + return nil + } + dep, ok := loaded.Config.Deployments[args[0]] + if !ok { + return nil + } + if repoNeed && dep.Repo != "" { + _ = fl.Set("repo", dep.Repo) + } + if pgNeed && dep.PGConnection != "" { + _ = fl.Set("pg-connection", dep.PGConnection) + } + return nil +} diff --git a/internal/cli/deployment_flag_resolution_test.go b/internal/cli/deployment_flag_resolution_test.go new file mode 100644 index 0000000..001d0ff --- /dev/null +++ b/internal/cli/deployment_flag_resolution_test.go @@ -0,0 +1,84 @@ +package cli_test + +import ( + "strings" + "testing" +) + +// (runCLI lives in listshowstatus_test.go: returns stdout, stderr, exit.) + +// TestDeploymentFlagResolution_Matrix proves the systemic fix (#12): every +// deployment-scoped command resolves --repo / --pg-connection from the +// named deployment in pg_hardstorage.yaml, instead of demanding the flag. +// Each command still fails afterwards (dead PG port / empty repo dir), but +// it must NOT fail with usage.missing_flag — that's the regression signal. +func TestDeploymentFlagResolution_Matrix(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + repoDir := t.TempDir() + // Port :1 is a closed port → connection refused immediately, so PG- + // touching commands fail fast and never hit a real local server. + t.Setenv("PG_HARDSTORAGE_CONFIG", + "deployments:\n mytest:\n"+ + " pg_connection: postgresql://postgres@127.0.0.1:1/postgres\n"+ + " repo: file://"+repoDir+"\n") + + target := t.TempDir() + "/restore" + cmds := [][]string{ + // MarkFlagRequired("repo") family: + {"list", "mytest"}, + {"status", "mytest"}, + {"show", "mytest", "someid"}, + {"hold", "list", "mytest"}, + {"rotate", "mytest"}, + {"recovery", "readiness", "mytest"}, + {"kms", "verify", "mytest"}, + {"wal", "list", "mytest"}, + {"wal", "audit", "mytest"}, + {"wal", "gaps", "mytest"}, + // MarkFlagRequired("pg-connection"): + {"wal", "preflight", "mytest"}, + // requireFlags() family (the originally-reported path): + {"backup", "mytest"}, + {"verify", "mytest", "latest"}, + {"restore", "mytest", "latest", "--target", target}, + } + for _, c := range cmds { + t.Run(strings.Join(c, "_"), func(t *testing.T) { + stdout, stderr, _ := runCLI(t, append(append([]string{}, c...), "-o", "json")...) + combined := stdout + stderr + if strings.Contains(combined, "usage.missing_flag") { + t.Fatalf("`pg_hardstorage %s` demanded a flag the configured deployment provides (#12):\n%s", + strings.Join(c, " "), combined) + } + }) + } +} + +// TestDeploymentFlagResolution_NoConfigStillRequires is the negative +// control: with no config and no flags, the requirement must still fire, +// so the resolver never papers over a genuine omission. +func TestDeploymentFlagResolution_NoConfigStillRequires(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("PG_HARDSTORAGE_ROOT", "") + t.Setenv("PG_HARDSTORAGE_CONFIG_DIR", "") + t.Setenv("PG_HARDSTORAGE_CONFIG", "") + + cmds := [][]string{ + {"list", "ghost"}, + {"status", "ghost"}, + {"wal", "preflight", "ghost"}, + {"backup", "ghost"}, + } + for _, c := range cmds { + t.Run(strings.Join(c, "_"), func(t *testing.T) { + stdout, stderr, _ := runCLI(t, append(append([]string{}, c...), "-o", "json")...) + combined := stdout + stderr + if !strings.Contains(combined, "usage.missing_flag") { + t.Fatalf("`pg_hardstorage %s` with no config/flag should still require the flag:\n%s", + strings.Join(c, " "), combined) + } + }) + } +} diff --git a/internal/cli/main_test.go b/internal/cli/main_test.go new file mode 100644 index 0000000..3219d69 --- /dev/null +++ b/internal/cli/main_test.go @@ -0,0 +1,30 @@ +package cli_test + +import ( + "os" + "path/filepath" + "testing" +) + +// TestMain isolates every CLI test from the developer's / CI runner's +// ambient ~/.config/pg_hardstorage. Now that deployment-scoped commands +// resolve --repo / --pg-connection from the deployment catalogue (#12), a +// real "db1" (or any) deployment present on the machine would otherwise +// satisfy those flags in tests that mean to assert the flag is required, +// making them pass or fail depending on the host. A clean, empty HOME +// makes the whole package deterministic; individual tests still inject +// their own config via t.Setenv as needed. +func TestMain(m *testing.M) { + home, err := os.MkdirTemp("", "hs-cli-test-home") + if err != nil { + panic(err) + } + os.Setenv("HOME", home) + os.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + os.Unsetenv("PG_HARDSTORAGE_CONFIG") + os.Unsetenv("PG_HARDSTORAGE_CONFIG_DIR") + os.Unsetenv("PG_HARDSTORAGE_ROOT") + code := m.Run() + _ = os.RemoveAll(home) + os.Exit(code) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 082ba79..70b0be4 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -119,7 +119,7 @@ sandbox-PG runtime — extend per docs/SPEC.md.`, // Order matters — running the dispatcher as root would // open the keyring + state dirs with root-owned files that // a later legitimate run as `pgbackup` couldn't read. - PersistentPreRunE: chainPreRunE(refuseRoot, installDispatcher), + PersistentPreRunE: chainPreRunE(refuseRoot, installDispatcher, resolveDeploymentDefaultsPreRun), } // Flag-parse failures (unknown flag, bad flag value) are usage From d7e81b66916f8d1280eefe6d06455124c1aac998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hans-J=C3=BCrgen=20Sch=C3=B6nig?= Date: Wed, 24 Jun 2026 21:40:33 +0200 Subject: [PATCH 4/4] docs(changelog): note the deployment-config resolution fix (#12) --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a940635..cd5571c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,19 @@ release config in CI. ## [Unreleased] +### Fix: deployment-scoped commands now read the deployment config (#12) + +`pg_hardstorage backup ` (and `restore`, `verify`, `list`, +`show`, `status`, `hold`, `rotate`, `recovery`, `repair`, `wal +preflight/stream/list/audit/prune/gaps`, `partial`, `kms verify/shred`, +…) used to demand `--pg-connection` / `--repo` even when the named +deployment already declared them in `pg_hardstorage.yaml`. They now +resolve those values from the deployment catalogue when the flags are +omitted (explicit flags still win); a deployment that isn't configured, +or a genuinely missing flag, still errors as before. Resolution happens +once, in a shared root pre-run hook, so every deployment-scoped command +behaves identically. + ### Packaging: remove the obsolete homebrew-formula.json manifest Dropped `scripts/homebrew-formula.json`, a leftover hand-maintained tap