From f859b256de84fc1a31921b62b0ef70cf9dd5090d Mon Sep 17 00:00:00 2001 From: Amolith Date: Tue, 31 Mar 2026 16:49:34 -0600 Subject: [PATCH] Add HasEnvSource to ResolvedConfig The snapshot loader was silently falling back to manual ID entry for presets that configure the repository via _COMMAND environ keys (e.g. RESTIC_REPOSITORY_COMMAND). This happened because hasRepoSource checked for RESTIC_REPOSITORY in the environ map but not for RESTIC_REPOSITORY_COMMAND, which keld resolves to RESTIC_REPOSITORY at execution time. Add HasEnvSource(key) to ResolvedConfig in the config package, which checks the environ map for both the direct key and the _COMMAND variant, plus the process environment as a fallback. This centralises the _COMMAND convention in one place so callers never need to remember it. Simplify hasRepoSource in the restic package to delegate to HasEnvSource for environ checks, replacing the hand-rolled map lookups and os.Getenv calls. --- internal/config/config.go | 31 +++++++++++ internal/config/resolved_test.go | 87 +++++++++++++++++++++++++++++++ internal/restic/list_snapshots.go | 20 ++----- 3 files changed, 121 insertions(+), 17 deletions(-) create mode 100644 internal/config/resolved_test.go diff --git a/internal/config/config.go b/internal/config/config.go index a4aa91988842fa54cf3be1cafc4148519c33c965..ef53cd5a21b7cc1789847df0d8aa39381171864d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -63,6 +63,37 @@ func (rc *ResolvedConfig) HasFlag(name string) bool { return false } +// commandSuffix is the suffix keld recognises in .environ keys to indicate +// that the value is a shell command whose stdout should be captured and +// used as the actual variable value (with the suffix stripped). +// +// For example, RESTIC_REPOSITORY_COMMAND = "op read ..." resolves to +// RESTIC_REPOSITORY at execution time. +const commandSuffix = "_COMMAND" + +// HasEnvSource reports whether the given environment variable will have +// a value when the restic process runs. It checks three sources in +// order: +// +// 1. Direct key in [ResolvedConfig.Environ] (e.g. RESTIC_REPOSITORY) +// 2. _COMMAND variant in Environ (e.g. RESTIC_REPOSITORY_COMMAND), +// which keld resolves before exec +// 3. Process environment via [os.Getenv] +// +// This centralises the _COMMAND convention so callers never need to +// check for it themselves. +func (rc *ResolvedConfig) HasEnvSource(key string) bool { + if rc.Environ != nil { + if _, ok := rc.Environ[key]; ok { + return true + } + if _, ok := rc.Environ[key+commandSuffix]; ok { + return true + } + } + return os.Getenv(key) != "" +} + // rawConfig is the entire parsed TOML file as nested string-keyed maps. type rawConfig map[string]any diff --git a/internal/config/resolved_test.go b/internal/config/resolved_test.go new file mode 100644 index 0000000000000000000000000000000000000000..e5da33f5401c88c613b09fd0e3a758bfaabcf717 --- /dev/null +++ b/internal/config/resolved_test.go @@ -0,0 +1,87 @@ +package config + +import ( + "testing" +) + +func TestHasEnvSource(t *testing.T) { + tests := []struct { + name string + environ map[string]string + envKey string + procEnv map[string]string // simulated process env + want bool + }{ + { + name: "direct key present", + environ: map[string]string{"RESTIC_REPOSITORY": "s3:bucket"}, + envKey: "RESTIC_REPOSITORY", + want: true, + }, + { + name: "command variant present", + environ: map[string]string{"RESTIC_REPOSITORY_COMMAND": "op read ..."}, + envKey: "RESTIC_REPOSITORY", + want: true, + }, + { + name: "neither direct nor command", + environ: map[string]string{"OTHER_VAR": "value"}, + envKey: "RESTIC_REPOSITORY", + want: false, + }, + { + name: "nil environ", + environ: nil, + envKey: "RESTIC_REPOSITORY", + want: false, + }, + { + name: "empty string value still counts as present", + environ: map[string]string{"RESTIC_REPOSITORY": ""}, + envKey: "RESTIC_REPOSITORY", + want: true, + }, + { + name: "command variant for non-restic key", + environ: map[string]string{"AWS_ACCESS_KEY_ID_COMMAND": "op read ..."}, + envKey: "AWS_ACCESS_KEY_ID", + want: true, + }, + { + name: "process env fallback", + environ: nil, + envKey: "KELD_TEST_HAS_ENV_SOURCE", + procEnv: map[string]string{"KELD_TEST_HAS_ENV_SOURCE": "from-process"}, + want: true, + }, + { + name: "process env empty when not set", + environ: nil, + envKey: "KELD_TEST_HAS_ENV_SOURCE", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Tests using t.Setenv cannot run in parallel. + if len(tt.procEnv) == 0 { + t.Parallel() + } + + for k, v := range tt.procEnv { + t.Setenv(k, v) + } + + cfg := &ResolvedConfig{ + Environ: tt.environ, + } + + got := cfg.HasEnvSource(tt.envKey) + if got != tt.want { + t.Errorf("HasEnvSource(%q) = %v, want %v", tt.envKey, got, tt.want) + } + }) + } +} diff --git a/internal/restic/list_snapshots.go b/internal/restic/list_snapshots.go index f3a927b97b43fba90f0a4d38bb5d8eced4e4bfdf..1312840343f7c7d1f7479fd7c8406fbbc001019f 100644 --- a/internal/restic/list_snapshots.go +++ b/internal/restic/list_snapshots.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "maps" - "os" "os/exec" "sort" @@ -51,8 +50,7 @@ var globalFlags = map[string]bool{ // hasRepoSource reports whether a repository location is available from // any source: CLI flags in the resolved config, the config's environ -// section, or the process environment (e.g. RESTIC_REPOSITORY already -// set in the user's shell). +// section (including _COMMAND variants), or the process environment. func hasRepoSource(cfg *config.ResolvedConfig) bool { for _, f := range cfg.Flags { switch f.Name { @@ -60,20 +58,8 @@ func hasRepoSource(cfg *config.ResolvedConfig) bool { return true } } - if cfg.Environ != nil { - if _, ok := cfg.Environ["RESTIC_REPOSITORY"]; ok { - return true - } - if _, ok := cfg.Environ["RESTIC_REPOSITORY_FILE"]; ok { - return true - } - } - // Check the process environment as a last resort — the user may - // have RESTIC_REPOSITORY set in their shell outside of keld config. - if os.Getenv("RESTIC_REPOSITORY") != "" || os.Getenv("RESTIC_REPOSITORY_FILE") != "" { - return true - } - return false + return cfg.HasEnvSource("RESTIC_REPOSITORY") || + cfg.HasEnvSource("RESTIC_REPOSITORY_FILE") } // buildSnapshotCmd constructs the argument vector for running