From d4ba764258ce23ec055c9a9ece35ce3970a49271 Mon Sep 17 00:00:00 2001 From: James Ainslie <42301770+jamesainslie@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:36:53 -0700 Subject: [PATCH] fix(config): make every config key environment-overridable DOT_ env overrides were driven by four hand-lists that had to agree (the BindEnv whitelist, the per-section env readers, the zero-value merge functions, and the struct itself) and did not: unlisted keys such as dotfile.package_name_mapping, symlinks.backup_dir, output.table_style, and the whole update and network sections were silently ignored, and the sparse-overlay merge could never propagate an env boolean set to false over a file value of true. Environment bindings are now derived by reflection from the config struct's mapstructure tags, and overrides are applied directly onto the loaded configuration under viper's IsSet, so explicit false survives. Path-typed values expand tilde and $VAR at apply time through the same ordered field list ExpandPaths uses. List values are comma-separated, matching the documented contract and the config set convention, where viper's own env handling would whitespace-split. A reflection-driven test walks every struct key and proves its env form lands, with a guard against vacuous samples, so a future key cannot ship silently unbound. Docs that enumerated the old partial binding list now state the uniform rule. Fixes #86 --- docs/user/04-configuration.md | 29 +-- docs/user/08-troubleshooting.md | 7 +- internal/config/expand.go | 30 ++- internal/config/loader.go | 390 +++++++++++++------------------- internal/config/loader_test.go | 180 +++++++++++++++ 5 files changed, 365 insertions(+), 271 deletions(-) diff --git a/docs/user/04-configuration.md b/docs/user/04-configuration.md index 1af47584..04c7a3f6 100644 --- a/docs/user/04-configuration.md +++ b/docs/user/04-configuration.md @@ -504,27 +504,14 @@ Environment variables use the prefix `DOT_` followed by the section-qualified ke by single underscores, uppercased. For example `symlinks.mode` becomes `DOT_SYMLINKS_MODE`. There is no double-underscore convention. -Only the following keys are bound: - -``` -DOT_DIRECTORIES_PACKAGE DOT_DIRECTORIES_TARGET DOT_DIRECTORIES_MANIFEST -DOT_LOGGING_LEVEL DOT_LOGGING_FORMAT DOT_LOGGING_DESTINATION DOT_LOGGING_FILE -DOT_SYMLINKS_MODE DOT_SYMLINKS_FOLDING DOT_SYMLINKS_OVERWRITE -DOT_SYMLINKS_BACKUP DOT_SYMLINKS_BACKUP_SUFFIX -DOT_IGNORE_USE_DEFAULTS DOT_IGNORE_PATTERNS DOT_IGNORE_OVERRIDES -DOT_IGNORE_PER_PACKAGE_IGNORE DOT_IGNORE_MAX_FILE_SIZE DOT_IGNORE_INTERACTIVE_LARGE_FILES -DOT_DOTFILE_TRANSLATE DOT_DOTFILE_PREFIX -DOT_OUTPUT_FORMAT DOT_OUTPUT_COLOR DOT_OUTPUT_PROGRESS -DOT_OUTPUT_VERBOSITY DOT_OUTPUT_WIDTH -DOT_OPERATIONS_DRY_RUN DOT_OPERATIONS_ATOMIC DOT_OPERATIONS_MAX_PARALLEL -DOT_PACKAGES_SORT_BY DOT_PACKAGES_AUTO_DISCOVER DOT_PACKAGES_VALIDATE_NAMES -DOT_DOCTOR_AUTO_FIX DOT_DOCTOR_CHECK_MANIFEST DOT_DOCTOR_CHECK_BROKEN_LINKS -DOT_DOCTOR_CHECK_ORPHANED DOT_DOCTOR_CHECK_PERMISSIONS -DOT_EXPERIMENTAL_PARALLEL DOT_EXPERIMENTAL_PROFILING -``` - -`symlinks.backup_dir`, `dotfile.package_name_mapping`, `output.table_style`, and every key under -`update` and `network` have no environment binding; set them in the configuration file. +Every configuration key in the tables above is bound: `DOT_` plus the uppercased key with `.` +replaced by `_`. `symlinks.backup_dir` becomes `DOT_SYMLINKS_BACKUP_DIR`, +`dotfile.package_name_mapping` becomes `DOT_DOTFILE_PACKAGE_NAME_MAPPING`, and the `update` and +`network` sections follow the same rule. An environment value overrides the configuration file even +when it sets a boolean to `false`. + +Path-typed values (`directories.*`, `symlinks.backup_dir`, `logging.file`) expand `~` and `$VAR` +references exactly as file values do. `DOT_CONFIG` is handled separately and overrides the configuration file path. diff --git a/docs/user/08-troubleshooting.md b/docs/user/08-troubleshooting.md index bb9c38f9..2366bcd6 100644 --- a/docs/user/08-troubleshooting.md +++ b/docs/user/08-troubleshooting.md @@ -310,9 +310,10 @@ dot manage dot-vim # Uses config file dot -v manage dot-vim # -v overrides config ``` -Not every key is bound to an environment variable. `symlinks.backup_dir`, -`dotfile.package_name_mapping`, `output.table_style`, and the whole `update` -and `network` sections have no environment binding. +Every configuration key is bound to an environment variable: `DOT_` plus the +uppercased key with dots replaced by underscores, including `symlinks.backup_dir`, +`dotfile.package_name_mapping`, `output.table_style`, and the `update` and +`network` sections. ### Invalid Configuration diff --git a/internal/config/expand.go b/internal/config/expand.go index 2c00a3a2..9b86bd5d 100644 --- a/internal/config/expand.go +++ b/internal/config/expand.go @@ -35,16 +35,7 @@ import ( // On error the configuration is left as written, and the message names both // the offending key and the reference that could not be resolved. func (c *ExtendedConfig) ExpandPaths() error { - fields := []struct { - key string - value *string - }{ - {"directories.package", &c.Directories.Package}, - {"directories.target", &c.Directories.Target}, - {"directories.manifest", &c.Directories.Manifest}, - {"symlinks.backup_dir", &c.Symlinks.BackupDir}, - {"logging.file", &c.Logging.File}, - } + fields := c.pathFields() expanded := make([]string, len(fields)) for i, field := range fields { @@ -62,6 +53,25 @@ func (c *ExtendedConfig) ExpandPaths() error { return nil } +// pathField pairs a path-typed configuration key with its value slot. +type pathField struct { + key string + value *string +} + +// pathFields returns every path-typed configuration value in a stable order. +// It is the single list both ExpandPaths and the environment override path +// consume, so a new path-typed field is expanded everywhere or nowhere. +func (c *ExtendedConfig) pathFields() []pathField { + return []pathField{ + {"directories.package", &c.Directories.Package}, + {"directories.target", &c.Directories.Target}, + {"directories.manifest", &c.Directories.Manifest}, + {"symlinks.backup_dir", &c.Symlinks.BackupDir}, + {"logging.file", &c.Logging.File}, + } +} + // expandPath expands environment variables and a leading tilde in a single // path value. An empty value stays empty, so callers can distinguish "unset" // from "set to the home directory". diff --git a/internal/config/loader.go b/internal/config/loader.go index 9aca242e..ce3eca8f 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "reflect" "strings" "github.com/spf13/viper" @@ -54,19 +55,14 @@ func (l *Loader) LoadWithEnv() (*ExtendedConfig, error) { return nil, err } - // Load from environment (sparse config with only env-set values) - envCfg := l.loadFromEnv() - - // Environment values carry the same "~" and "$VAR" forms a config file may - // use. Expand the overlay before merging, so file values that were already - // expanded on load are not run through expansion a second time. - if err := envCfg.ExpandPaths(); err != nil { - return nil, fmt.Errorf("expand environment configuration: %w", err) + // Apply environment overrides directly onto the loaded configuration, + // keyed on viper's IsSet. A sparse overlay merged by zero-value cannot + // represent "explicitly set to false", which silently discarded env + // overrides like DOT_DOTFILE_PACKAGE_NAME_MAPPING=false (issue #86). + if err := applyEnvOverrides(l.newEnvViper(), cfg); err != nil { + return nil, fmt.Errorf("apply environment configuration: %w", err) } - // Use simple merge for env (only strings, no booleans unless tracked) - cfg = mergeConfigs(cfg, envCfg) - // Validate merged configuration if err := cfg.Validate(); err != nil { return nil, fmt.Errorf("invalid configuration: %w", err) @@ -104,230 +100,166 @@ func (l *Loader) LoadWithFlags(flags map[string]interface{}) (*ExtendedConfig, e return cfg, nil } -// loadFromEnv loads configuration from environment variables. -// Returns a sparse config with only explicitly set environment values. -func (l *Loader) loadFromEnv() *ExtendedConfig { +// newEnvViper returns a viper instance with every configuration key bound +// to its DOT_-prefixed environment variable. +func (l *Loader) newEnvViper() *viper.Viper { v := viper.New() - - // Set up environment variable handling v.SetEnvPrefix(strings.ToUpper(l.appName)) v.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) v.AutomaticEnv() - // Bind all configuration keys - l.bindEnvKeys(v) - - // Create sparse config - cfg := createSparseConfig() - - // Load each section - loadDirectoriesFromEnv(v, &cfg.Directories) - loadLoggingFromEnv(v, &cfg.Logging) - loadSymlinksFromEnv(v, &cfg.Symlinks) - loadIgnoreFromEnv(v, &cfg.Ignore) - loadDotfileFromEnv(v, &cfg.Dotfile) - loadOutputFromEnv(v, &cfg.Output) - loadOperationsFromEnv(v, &cfg.Operations) - loadPackagesFromEnv(v, &cfg.Packages) - loadDoctorFromEnv(v, &cfg.Doctor) - loadExperimentalFromEnv(v, &cfg.Experimental) - - return cfg -} - -func loadDirectoriesFromEnv(v *viper.Viper, cfg *DirectoriesConfig) { - if v.IsSet("directories.package") { - cfg.Package = v.GetString("directories.package") - } - if v.IsSet("directories.target") { - cfg.Target = v.GetString("directories.target") + // Bind every key, derived from the config struct's mapstructure tags so + // a new field cannot be forgotten here (issue #86: a hand-written bind + // list silently ignored the keys it missed). + for _, key := range configKeys() { + _ = v.BindEnv(key) // BindEnv only errors when called with no key } - if v.IsSet("directories.manifest") { - cfg.Manifest = v.GetString("directories.manifest") - } -} -func loadLoggingFromEnv(v *viper.Viper, cfg *LoggingConfig) { - if v.IsSet("logging.level") { - cfg.Level = v.GetString("logging.level") - } - if v.IsSet("logging.format") { - cfg.Format = v.GetString("logging.format") - } - if v.IsSet("logging.destination") { - cfg.Destination = v.GetString("logging.destination") - } - if v.IsSet("logging.file") { - cfg.File = v.GetString("logging.file") - } + return v } -func loadSymlinksFromEnv(v *viper.Viper, cfg *SymlinksConfig) { - if v.IsSet("symlinks.mode") { - cfg.Mode = v.GetString("symlinks.mode") - } - if v.IsSet("symlinks.folding") { - cfg.Folding = v.GetBool("symlinks.folding") - } - if v.IsSet("symlinks.overwrite") { - cfg.Overwrite = v.GetBool("symlinks.overwrite") - } - if v.IsSet("symlinks.backup") { - cfg.Backup = v.GetBool("symlinks.backup") - } - if v.IsSet("symlinks.backup_suffix") { - cfg.BackupSuffix = v.GetString("symlinks.backup_suffix") - } -} - -func loadIgnoreFromEnv(v *viper.Viper, cfg *IgnoreConfig) { - if v.IsSet("ignore.use_defaults") { - cfg.UseDefaults = v.GetBool("ignore.use_defaults") - } - if v.IsSet("ignore.patterns") { - cfg.Patterns = v.GetStringSlice("ignore.patterns") - } - if v.IsSet("ignore.overrides") { - cfg.Overrides = v.GetStringSlice("ignore.overrides") - } - if v.IsSet("ignore.per_package_ignore") { - cfg.PerPackageIgnore = v.GetBool("ignore.per_package_ignore") - } - if v.IsSet("ignore.max_file_size") { - cfg.MaxFileSize = v.GetInt64("ignore.max_file_size") - } - if v.IsSet("ignore.interactive_large_files") { - cfg.InteractiveLargeFiles = v.GetBool("ignore.interactive_large_files") - } -} - -func loadDotfileFromEnv(v *viper.Viper, cfg *DotfileConfig) { - if v.IsSet("dotfile.translate") { - cfg.Translate = v.GetBool("dotfile.translate") - } - if v.IsSet("dotfile.prefix") { - cfg.Prefix = v.GetString("dotfile.prefix") - } -} - -func loadOutputFromEnv(v *viper.Viper, cfg *OutputConfig) { - if v.IsSet("output.format") { - cfg.Format = v.GetString("output.format") - } - if v.IsSet("output.color") { - cfg.Color = v.GetString("output.color") - } - if v.IsSet("output.progress") { - cfg.Progress = v.GetBool("output.progress") - } - if v.IsSet("output.verbosity") { - cfg.Verbosity = v.GetInt("output.verbosity") - } - if v.IsSet("output.width") { - cfg.Width = v.GetInt("output.width") - } -} - -func loadOperationsFromEnv(v *viper.Viper, cfg *OperationsConfig) { - if v.IsSet("operations.dry_run") { - cfg.DryRun = v.GetBool("operations.dry_run") - } - if v.IsSet("operations.atomic") { - cfg.Atomic = v.GetBool("operations.atomic") - } - if v.IsSet("operations.max_parallel") { - cfg.MaxParallel = v.GetInt("operations.max_parallel") - } -} - -func loadPackagesFromEnv(v *viper.Viper, cfg *PackagesConfig) { - if v.IsSet("packages.sort_by") { - cfg.SortBy = v.GetString("packages.sort_by") - } - if v.IsSet("packages.auto_discover") { - cfg.AutoDiscover = v.GetBool("packages.auto_discover") +// configKeys returns every section.field key in ExtendedConfig, derived by +// reflection from the mapstructure tags. Only tagged struct sections with +// tagged fields produce keys, so the walk tolerates the struct growing a +// scalar or untagged field without panicking on every command start. +func configKeys() []string { + var keys []string + root := reflect.TypeOf(ExtendedConfig{}) + for i := 0; i < root.NumField(); i++ { + sectionType := root.Field(i).Type + if sectionType.Kind() != reflect.Struct { + continue + } + section := root.Field(i).Tag.Get("mapstructure") + if section == "" { + continue + } + for j := 0; j < sectionType.NumField(); j++ { + field := sectionType.Field(j).Tag.Get("mapstructure") + if field == "" { + continue + } + keys = append(keys, section+"."+field) + } } - if v.IsSet("packages.validate_names") { - cfg.ValidateNames = v.GetBool("packages.validate_names") + return keys +} + +// applyEnvOverrides applies every environment-set key directly onto cfg. +// Path-typed values are expanded here, mirroring the expansion file values +// receive on load. The per-section lists below are kept total by +// TestLoader_EnvOverridesEveryConfigKey, which walks the config struct. +func applyEnvOverrides(v *viper.Viper, cfg *ExtendedConfig) error { + if err := applyPathEnv(v, cfg); err != nil { + return err + } + + applyEnv(v, v.GetString, map[string]*string{ + "logging.level": &cfg.Logging.Level, + "logging.format": &cfg.Logging.Format, + "logging.destination": &cfg.Logging.Destination, + "symlinks.mode": &cfg.Symlinks.Mode, + "symlinks.backup_suffix": &cfg.Symlinks.BackupSuffix, + "dotfile.prefix": &cfg.Dotfile.Prefix, + "output.format": &cfg.Output.Format, + "output.color": &cfg.Output.Color, + "output.table_style": &cfg.Output.TableStyle, + "packages.sort_by": &cfg.Packages.SortBy, + "update.package_manager": &cfg.Update.PackageManager, + "update.repository": &cfg.Update.Repository, + "network.http_proxy": &cfg.Network.HTTPProxy, + "network.https_proxy": &cfg.Network.HTTPSProxy, + "network.no_proxy": &cfg.Network.NoProxy, + }) + + applyEnv(v, v.GetBool, map[string]*bool{ + "symlinks.folding": &cfg.Symlinks.Folding, + "symlinks.overwrite": &cfg.Symlinks.Overwrite, + "symlinks.backup": &cfg.Symlinks.Backup, + "ignore.use_defaults": &cfg.Ignore.UseDefaults, + "ignore.per_package_ignore": &cfg.Ignore.PerPackageIgnore, + "ignore.interactive_large_files": &cfg.Ignore.InteractiveLargeFiles, + "dotfile.translate": &cfg.Dotfile.Translate, + "dotfile.package_name_mapping": &cfg.Dotfile.PackageNameMapping, + "output.progress": &cfg.Output.Progress, + "operations.dry_run": &cfg.Operations.DryRun, + "operations.atomic": &cfg.Operations.Atomic, + "packages.auto_discover": &cfg.Packages.AutoDiscover, + "packages.validate_names": &cfg.Packages.ValidateNames, + "doctor.auto_fix": &cfg.Doctor.AutoFix, + "doctor.check_manifest": &cfg.Doctor.CheckManifest, + "doctor.check_broken_links": &cfg.Doctor.CheckBrokenLinks, + "doctor.check_orphaned": &cfg.Doctor.CheckOrphaned, + "doctor.check_permissions": &cfg.Doctor.CheckPermissions, + "update.check_on_startup": &cfg.Update.CheckOnStartup, + "update.include_prerelease": &cfg.Update.IncludePrerelease, + "experimental.parallel": &cfg.Experimental.Parallel, + "experimental.profiling": &cfg.Experimental.Profiling, + }) + + applyEnv(v, v.GetInt, map[string]*int{ + "output.verbosity": &cfg.Output.Verbosity, + "output.width": &cfg.Output.Width, + "operations.max_parallel": &cfg.Operations.MaxParallel, + "update.check_frequency": &cfg.Update.CheckFrequency, + "network.timeout": &cfg.Network.Timeout, + "network.connect_timeout": &cfg.Network.ConnectTimeout, + "network.tls_timeout": &cfg.Network.TLSTimeout, + }) + + applyEnv(v, v.GetInt64, map[string]*int64{ + "ignore.max_file_size": &cfg.Ignore.MaxFileSize, + }) + + // Lists are comma-separated in the environment, matching the documented + // contract and the comma convention config set already uses. Viper's own + // GetStringSlice would whitespace-split instead. + applyEnv(v, func(key string) []string { return splitEnvList(v.GetString(key)) }, map[string]*[]string{ + "ignore.patterns": &cfg.Ignore.Patterns, + "ignore.overrides": &cfg.Ignore.Overrides, + }) + + return nil +} + +// applyPathEnv sets path-typed values from the environment, expanding "~" +// and "$VAR" references the way file values are expanded on load. It walks +// pathFields in order, so with several bad values the reported key is +// deterministic. +func applyPathEnv(v *viper.Viper, cfg *ExtendedConfig) error { + for _, field := range cfg.pathFields() { + if !v.IsSet(field.key) { + continue + } + expanded, err := expandPath(v.GetString(field.key)) + if err != nil { + return fmt.Errorf("%s: %w", field.key, err) + } + *field.value = expanded } + return nil } -func loadDoctorFromEnv(v *viper.Viper, cfg *DoctorConfig) { - if v.IsSet("doctor.auto_fix") { - cfg.AutoFix = v.GetBool("doctor.auto_fix") - } - if v.IsSet("doctor.check_manifest") { - cfg.CheckManifest = v.GetBool("doctor.check_manifest") - } - if v.IsSet("doctor.check_broken_links") { - cfg.CheckBrokenLinks = v.GetBool("doctor.check_broken_links") - } - if v.IsSet("doctor.check_orphaned") { - cfg.CheckOrphaned = v.GetBool("doctor.check_orphaned") - } - if v.IsSet("doctor.check_permissions") { - cfg.CheckPermissions = v.GetBool("doctor.check_permissions") +// applyEnv copies every environment-set key in targets onto its destination. +func applyEnv[T any](v *viper.Viper, get func(string) T, targets map[string]*T) { + for key, dst := range targets { + if v.IsSet(key) { + *dst = get(key) + } } } -func loadExperimentalFromEnv(v *viper.Viper, cfg *ExperimentalConfig) { - if v.IsSet("experimental.parallel") { - cfg.Parallel = v.GetBool("experimental.parallel") - } - if v.IsSet("experimental.profiling") { - cfg.Profiling = v.GetBool("experimental.profiling") +// splitEnvList splits a comma-separated environment value into its items, +// trimming surrounding whitespace and dropping empty entries. +func splitEnvList(value string) []string { + parts := strings.Split(value, ",") + items := make([]string, 0, len(parts)) + for _, part := range parts { + if trimmed := strings.TrimSpace(part); trimmed != "" { + items = append(items, trimmed) + } } -} - -// bindEnvKeys binds all configuration keys to environment variables. -func (l *Loader) bindEnvKeys(v *viper.Viper) { - v.BindEnv("directories.package") - v.BindEnv("directories.target") - v.BindEnv("directories.manifest") - - v.BindEnv("logging.level") - v.BindEnv("logging.format") - v.BindEnv("logging.destination") - v.BindEnv("logging.file") - - v.BindEnv("symlinks.mode") - v.BindEnv("symlinks.folding") - v.BindEnv("symlinks.overwrite") - v.BindEnv("symlinks.backup") - v.BindEnv("symlinks.backup_suffix") - - v.BindEnv("ignore.use_defaults") - v.BindEnv("ignore.patterns") - v.BindEnv("ignore.overrides") - v.BindEnv("ignore.per_package_ignore") - v.BindEnv("ignore.max_file_size") - v.BindEnv("ignore.interactive_large_files") - - v.BindEnv("dotfile.translate") - v.BindEnv("dotfile.prefix") - - v.BindEnv("output.format") - v.BindEnv("output.color") - v.BindEnv("output.progress") - v.BindEnv("output.verbosity") - v.BindEnv("output.width") - - v.BindEnv("operations.dry_run") - v.BindEnv("operations.atomic") - v.BindEnv("operations.max_parallel") - - v.BindEnv("packages.sort_by") - v.BindEnv("packages.auto_discover") - v.BindEnv("packages.validate_names") - - v.BindEnv("doctor.auto_fix") - v.BindEnv("doctor.check_manifest") - v.BindEnv("doctor.check_broken_links") - v.BindEnv("doctor.check_orphaned") - v.BindEnv("doctor.check_permissions") - - v.BindEnv("experimental.parallel") - v.BindEnv("experimental.profiling") + return items } // configFromFlags creates partial config from flag map. @@ -339,20 +271,10 @@ func (l *Loader) configFromFlags(flags map[string]interface{}) (*ExtendedConfig, return cfg, verbositySet } -// createSparseConfig creates an empty config for flag/env merging. +// createSparseConfig creates an empty config for flag merging. Verbosity -1 +// is the "not set" sentinel. func createSparseConfig() *ExtendedConfig { - return &ExtendedConfig{ - Directories: DirectoriesConfig{}, - Logging: LoggingConfig{}, - Symlinks: SymlinksConfig{}, - Ignore: IgnoreConfig{}, - Dotfile: DotfileConfig{}, - Output: OutputConfig{Verbosity: -1}, // Use -1 as sentinel for "not set" - Operations: OperationsConfig{}, - Packages: PackagesConfig{}, - Doctor: DoctorConfig{}, - Experimental: ExperimentalConfig{}, - } + return &ExtendedConfig{Output: OutputConfig{Verbosity: -1}} } // applyFlagsToConfig maps command-line flags to configuration fields. @@ -413,12 +335,6 @@ func applyOutputFlags(cfg *ExtendedConfig, flags map[string]interface{}) bool { return verbositySet } -// mergeConfigs merges two configs, with override taking precedence for non-zero values. -// Only merges fields that are explicitly set in override (non-empty strings, non-zero lists). -func mergeConfigs(base, override *ExtendedConfig) *ExtendedConfig { - return mergeConfigsWithVerbosity(base, override, false) -} - // mergeConfigsWithVerbosity merges configs with special handling for verbosity. func mergeConfigsWithVerbosity(base, override *ExtendedConfig, verbosityExplicit bool) *ExtendedConfig { merged := *base diff --git a/internal/config/loader_test.go b/internal/config/loader_test.go index 7ce745b9..56f7ad3e 100644 --- a/internal/config/loader_test.go +++ b/internal/config/loader_test.go @@ -3,6 +3,9 @@ package config_test import ( "os" "path/filepath" + "reflect" + "strconv" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -357,3 +360,180 @@ package = "/test/dotfiles" }) } } + +// envSampleValues holds one valid, non-default sample per configuration key, +// in the string form an operator would export. TestLoader_EnvOverridesEveryConfigKey +// walks the ExtendedConfig struct, so a new key without a sample here fails +// loudly instead of shipping as a silently ignored env var (issue #86). +var envSampleValues = map[string]string{ + "directories.package": "/env/pkgs", + "directories.target": "/env/target", + "directories.manifest": "/env/manifest", + "logging.level": "DEBUG", + "logging.format": "json", + "logging.destination": "stdout", + "logging.file": "/env/dot.log", + "symlinks.mode": "absolute", + "symlinks.folding": "false", + "symlinks.overwrite": "true", + "symlinks.backup": "true", + "symlinks.backup_suffix": ".orig", + "symlinks.backup_dir": "/env/backups", + "ignore.use_defaults": "false", + "ignore.patterns": "*.log", + "ignore.overrides": "keep.me", + "ignore.per_package_ignore": "false", + "ignore.max_file_size": "1024", + "ignore.interactive_large_files": "false", + "dotfile.translate": "false", + "dotfile.prefix": "custom-", + "dotfile.package_name_mapping": "false", + "output.format": "json", + "output.color": "always", + "output.table_style": "simple", + "output.progress": "false", + "output.verbosity": "2", + "output.width": "120", + "operations.dry_run": "true", + "operations.atomic": "false", + "operations.max_parallel": "4", + "packages.sort_by": "links", + "packages.auto_discover": "false", + "packages.validate_names": "false", + "doctor.auto_fix": "true", + "doctor.check_manifest": "false", + "doctor.check_broken_links": "false", + "doctor.check_orphaned": "false", + "doctor.check_permissions": "false", + "update.check_on_startup": "false", + "update.check_frequency": "48", + "update.package_manager": "brew", + "update.repository": "acme/dot", + "update.include_prerelease": "true", + "network.http_proxy": "http://proxy.env:3128", + "network.https_proxy": "http://proxy.env:3129", + "network.no_proxy": "localhost", + "network.timeout": "30", + "network.connect_timeout": "9", + "network.tls_timeout": "7", + "experimental.parallel": "true", + "experimental.profiling": "true", +} + +// configLeafKeys walks the two-level ExtendedConfig shape and returns every +// section.field key together with the reflect.Value accessor for a loaded +// config, so the test cannot drift from the struct. +func configLeafKeys(t *testing.T, cfg *config.ExtendedConfig) map[string]reflect.Value { + t.Helper() + + keys := make(map[string]reflect.Value) + root := reflect.ValueOf(cfg).Elem() + rootType := root.Type() + + for i := 0; i < rootType.NumField(); i++ { + section := rootType.Field(i).Tag.Get("mapstructure") + require.NotEmpty(t, section, "field %s has no mapstructure tag", rootType.Field(i).Name) + + sectionValue := root.Field(i) + sectionType := sectionValue.Type() + for j := 0; j < sectionType.NumField(); j++ { + field := sectionType.Field(j).Tag.Get("mapstructure") + require.NotEmpty(t, field, "field %s.%s has no mapstructure tag", section, sectionType.Field(j).Name) + keys[section+"."+field] = sectionValue.Field(j) + } + } + + return keys +} + +func TestLoader_EnvOverridesEveryConfigKey(t *testing.T) { + // Issue #86: DOT_ env overrides were driven by hand-lists, so keys + // outside them (dotfile.package_name_mapping among others) were + // silently ignored. Every key in the config struct must be settable + // from the environment. + for key := range configLeafKeys(t, config.DefaultExtended()) { + t.Run(key, func(t *testing.T) { + sample, ok := envSampleValues[key] + require.True(t, ok, "config key %s has no env sample; add one to envSampleValues", key) + + envVar := "DOT_" + strings.ToUpper(strings.ReplaceAll(key, ".", "_")) + t.Setenv(envVar, sample) + + loader := config.NewLoader("dot", filepath.Join(t.TempDir(), "missing.yaml")) + cfg, err := loader.LoadWithEnv() + require.NoError(t, err) + + got := configLeafKeys(t, cfg)[key] + + var want interface{} + switch got.Kind() { + case reflect.Bool: + b, perr := strconv.ParseBool(sample) + require.NoError(t, perr) + want = b + case reflect.Int: + n, perr := strconv.Atoi(sample) + require.NoError(t, perr) + want = n + case reflect.Int64: + n, perr := strconv.ParseInt(sample, 10, 64) + require.NoError(t, perr) + want = n + case reflect.String: + want = sample + case reflect.Slice: + want = []string{sample} + default: + t.Fatalf("unhandled kind %s for %s", got.Kind(), key) + } + + // A sample equal to the default would make this subtest pass + // even if the override were silently dropped. + def := configLeafKeys(t, config.DefaultExtended())[key] + require.NotEqual(t, def.Interface(), want, + "sample for %s equals the default; pick a distinct value in envSampleValues", key) + + assert.Equal(t, want, got.Interface(), "%s not overridden by %s", key, envVar) + }) + } +} + +func TestLoader_EnvListValuesAreCommaSeparated(t *testing.T) { + // The documented contract (and the config set convention) is + // comma-separated lists; viper's own env handling would split on + // whitespace instead, turning "*.log,*.tmp" into one bogus pattern. + t.Setenv("DOT_IGNORE_PATTERNS", "*.log, *.tmp,cache") + + loader := config.NewLoader("dot", filepath.Join(t.TempDir(), "missing.yaml")) + cfg, err := loader.LoadWithEnv() + require.NoError(t, err) + + assert.Equal(t, []string{"*.log", "*.tmp", "cache"}, cfg.Ignore.Patterns) +} + +func TestLoader_EnvFalseOverridesFileTrue(t *testing.T) { + // The old sparse-config merge could only propagate true booleans, so an + // env var set to false could never override a file value of true. That + // is the exact shape of the issue #86 report. + configPath := filepath.Join(t.TempDir(), "config.yaml") + content := ` +dotfile: + translate: true + package_name_mapping: true +symlinks: + folding: true +` + require.NoError(t, os.WriteFile(configPath, []byte(content), 0o600)) + + t.Setenv("DOT_DOTFILE_PACKAGE_NAME_MAPPING", "false") + t.Setenv("DOT_DOTFILE_TRANSLATE", "false") + t.Setenv("DOT_SYMLINKS_FOLDING", "false") + + loader := config.NewLoader("dot", configPath) + cfg, err := loader.LoadWithEnv() + require.NoError(t, err) + + assert.False(t, cfg.Dotfile.PackageNameMapping, "env false must override file true") + assert.False(t, cfg.Dotfile.Translate, "env false must override file true") + assert.False(t, cfg.Symlinks.Folding, "env false must override file true") +}