fix(cli): deployment-scoped commands resolve --repo/--pg-connection from config (closes #12)

CHANGELOG.md

@@ -50,6 +50,19 @@ release config in CI.
 
 ## [Unreleased]
 
+### Fix: deployment-scoped commands now read the deployment config (#12)
+
+`pg_hardstorage backup <deployment>` (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

internal/cli/backup.go

@@ -170,13 +170,23 @@ 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 <deployment>` 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 (#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:
+	// 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

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 <deployment>` 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

internal/cli/deployment_defaults.go

@@ -0,0 +1,106 @@
+// deployment_defaults.go — shared resolution of connection/repo defaults
+// from the deployment catalogue in pg_hardstorage.yaml.
+//
+// Commands that take a <deployment> 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/spf13/cobra"
+
+	"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)
+}
+
+// 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
+}

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)
+	}
+}

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)
+			}
+		})
+	}
+}

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)
+}