cleanup: use viper to handle config priority - #711
Conversation
Signed-off-by: Lionel Villard <villard@us.ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the configuration loading system to use the Viper library instead of custom precedence logic. The change simplifies configuration management by replacing the manual StaticConfigFlags struct and precedence helper functions with Viper's built-in configuration resolution.
Changes:
- Replaced
StaticConfigFlagsstruct with*flag.FlagSetparameter inconfig.Load() - Implemented Viper-based configuration loading with proper precedence: flags > env > ConfigMap > defaults
- Removed ~150 lines of custom precedence helper functions (
getBoolValue,getDurationValue, etc.) - Updated all tests to use the new flag-based API
- Added comprehensive documentation explaining the new precedence model
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/config/loader.go |
Complete refactor to use Viper for configuration loading; removed StaticConfigFlags struct and helper functions |
internal/config/loader_test.go |
Updated all tests to create FlagSets instead of StaticConfigFlags structs |
internal/config/config.go |
Updated test helper to pass nil instead of empty StaticConfigFlags |
internal/controller/predicates_test.go |
Updated test calls to pass nil instead of empty StaticConfigFlags |
cmd/main.go |
Simplified by removing StaticConfigFlags construction, now passes flag.CommandLine directly |
docs/user-guide/configuration.md |
Updated documentation with clearer precedence explanation and comprehensive configuration tables |
docs/developer-guide/configuration.md |
New developer guide section documenting the unified configuration system |
go.mod, go.sum |
Added Viper and related dependencies; upgraded pflag from 1.0.7 to 1.0.10 |
Signed-off-by: Lionel Villard <villard@us.ibm.com>
mamy-CS
left a comment
mamy-CS
left a comment
There was a problem hiding this comment.
approving with some comments /lgtm
| } | ||
| v.SetConfigType("dotenv") | ||
| if err := v.ReadConfig(buf); err != nil { | ||
| ctrl.Log.Info("Failed to parse ConfigMap data into viper", "error", err) |
There was a problem hiding this comment.
return an error here, maybe fail fast on parsing errors.
There was a problem hiding this comment.
the following PR will remove this code.
| if flagSet != nil { | ||
| for viperKey, flagName := range flagBindings { | ||
| if f := flagSet.Lookup(flagName); f != nil { | ||
| _ = v.BindPFlag(viperKey, f) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding logging when a flag is not found here
There was a problem hiding this comment.
this code will be removed in a follow-up PR. There is a way to bind the entire commandLine.
| var flagBindings = map[string]string{ | ||
| "METRICS_BIND_ADDRESS": "metrics-bind-address", | ||
| "HEALTH_PROBE_BIND_ADDRESS": "health-probe-bind-address", | ||
| "LEADER_ELECT": "leader-elect", | ||
| "LEADER_ELECTION_LEASE_DURATION": "leader-election-lease-duration", | ||
| "LEADER_ELECTION_RENEW_DEADLINE": "leader-election-renew-deadline", | ||
| "LEADER_ELECTION_RETRY_PERIOD": "leader-election-retry-period", | ||
| "REST_CLIENT_TIMEOUT": "rest-client-timeout", | ||
| "METRICS_SECURE": "metrics-secure", | ||
| "ENABLE_HTTP2": "enable-http2", | ||
| "WATCH_NAMESPACE": "watch-namespace", | ||
| "V": "v", | ||
| "WEBHOOK_CERT_PATH": "webhook-cert-path", | ||
| "WEBHOOK_CERT_NAME": "webhook-cert-name", | ||
| "WEBHOOK_CERT_KEY": "webhook-cert-key", | ||
| "METRICS_CERT_PATH": "metrics-cert-path", | ||
| "METRICS_CERT_NAME": "metrics-cert-name", | ||
| "METRICS_CERT_KEY": "metrics-cert-key", | ||
| } |
There was a problem hiding this comment.
this flagBindings map must be manually kept in sync with flag definitions in main.go. This could be error-prone. maybe add a test that validates all entries in flagBindings exist in the actual flag set?
There was a problem hiding this comment.
this table will be removed in a follow-up PR.
| } | ||
|
|
||
| // Load unified configuration (fail-fast if invalid) | ||
| // Viper resolves precedence: flags > env > ConfigMap > defaults |
There was a problem hiding this comment.
maybe clarify here that only explicitly-set flags take precedence
There was a problem hiding this comment.
good idea, I'll provide a link to the doc.
|
merging as the failing is most likely due to #702 |
No description provided.