-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use config defaults with a custom config #392
Conversation
Here are the logs:
|
also of note, retrieve works and there is data despite the errors in the logs. I'll file an issue to look into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to just merge this and follow on with an issue + PR to add a test to ensure this behavior.
} | ||
|
||
// Set defaults for the config.Config and overwrite fields with the passed in config. | ||
mergo.MergeWithOverwrite(cfg.Config, config.New()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this was inbound from the customer I think we should write some tests to ensure behavior.
Start with sensible defaults instead of zero values. If a config is passed in it will merge into the default config instead of a zero-value config. Fixes vmware-tanzu#390 Signed-off-by: Chuck Ha <chuck@heptio.com>
Adding tests, will push up another pr in a few. yay jetlag -_- |
(I closed this because I had discovered a bug while writing tests and didn't want an accidental merge 🙃) |
Start with sensible defaults instead of zero values. If a config
is passed in it will merge into the default config instead of
a zero-value config.
Fixes #390
Signed-off-by: Chuck Ha chuck@heptio.com
What this PR does / why we need it:
This uses the default config (not the zero value config) as the base config. If no config is supplied, we end up with default values, if a config is supplied we end up with the default values overwritten by any supplied config fields.
Which issue(s) this PR fixes
Special notes for your reviewer:
We already have mergo in our dependency list!
Release note: