-
Notifications
You must be signed in to change notification settings - Fork 3
Allow to read config also from environment #14
Conversation
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.
Thanks for doing this! However, it doesn't seem to work for me:
$ env NAMESPACE=ns LOGGREGATOR-ENDPOINT=foo LOGGREGATOR-CA-PATH=bar LOGGREGATOR-CERT-PATH=baz LOGGREGATOR-KEY-PATH=quux ./binaries/eirini-loggregator-bridge
Namespace: ns
Loggregator-endpoint: foo
Loggregator-ca-path: bar
Loggregator-cert-path: baz
Loggregator-key-path: quux
namespace is missing from configuration
While the debugging bits seem to get the settings, the viper.Unmarshal
didn't seem to. Note that I didn't have a --config
anywhere.
Thanks for the review! you are right, I think we are hitting spf13/viper#188 , I'll have a look! |
I think I've got it:
I've also changed the debug lines to print the data from the config, so it's more easy to spot issues |
WIP: doing this aside with the InitContainer. The InitContainer can be dropped once cloudfoundry-incubator/eirini-loggregator-bridge#14 is merged.
cmd/root.go
Outdated
@@ -63,18 +66,45 @@ func init() { | |||
rootCmd.PersistentFlags().StringVar(&kubeconfig, "kubeconfig", "", "kubeconfig file path. This is optional, in cluster config will be used if not set") | |||
} | |||
|
|||
// See: https://github.com/spf13/viper/issues/188#issuecomment-399884438 |
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.
This code is copied from a GitHub comment; what license is it under? We may need to rewrite it instead. (Also, I find the variable names to be opaque.)
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.
Right, but otoh it won't get much different if we end up using reflection. I'll try a different approach then
cmd/root.go
Outdated
t := ift.Field(i) | ||
tv, ok := t.Tag.Lookup("mapstructure") | ||
if !ok { | ||
continue |
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.
If I read the mapstructure docs correctly, this is doing the wrong thing: if the tag is missing, it should by default use the field name. Also, the tag may contain suffixes like ,squash
and ,remain
, so we should strip anything after a comma.
Alternatively, we can actually import mapstructure
, and convert an instance of Config
to JSON/YAML and have viper load that before loading the real config, according to a different comment in that issue.
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.
If I read the mapstructure docs correctly, this is doing the wrong thing: if the tag is missing, it should by default use the field name. Also, the tag may contain suffixes like
,squash
and,remain
, so we should strip anything after a comma.
Don't see the use case here, and the code would grow too much of complexity for a very tiny improvement that will make it even more fragile. I prefer to look after another way of doing it, avoding reflection at all ( I was unattracted by this solution as well, too ! )
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've been playing around with spf13/viper#188 (comment) here b1c8bd7 without luck - it doesn't seem to work anymore, and moreover forces to ship "defaults" which we tried to avoid here, prefering validation of proper input ( to avoid surprises during configuration )
WIP: it's not working yet
cmd/root.go
Outdated
} | ||
|
||
emptyConfigReader := bytes.NewReader(emptyConfigBytes) | ||
viper.MergeConfig(emptyConfigReader) |
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.
If I'm reading that viper bug correctly, the MergeConfig
call needs to be done even if cfgFile == ""
.
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've tried already few times as well (also by dropping any check to cfgFile) , I just ended up defining the binds manually: 32059c9
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.
See spf13/viper#761 to check how Unmarshal in Viper works under the hood
If looks good, feel free to squash when merging, no need for the whole history ( I kept it here for reference ) |
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.
Thanks! Sad that viper doesn't seem to work as well as we'd like, and we need the workarounds… ah well.
I'm happy with squashing as well, but I'll let you choose if you want to take the suggestions first.
viper.SetDefault("NAMESPACE", "") | ||
viper.SetDefault("LOGGREGATOR_KEY_PATH", "") | ||
viper.SetDefault("LOGGREGATOR_ENDPOINT", "") | ||
viper.SetDefault("LOGGREGATOR_CA_PATH", "") | ||
viper.SetDefault("LOGGREGATOR_CERT_PATH", "") |
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 don't think we actually need these (since they're the zero values already).
viper.SetDefault("NAMESPACE", "") | |
viper.SetDefault("LOGGREGATOR_KEY_PATH", "") | |
viper.SetDefault("LOGGREGATOR_ENDPOINT", "") | |
viper.SetDefault("LOGGREGATOR_CA_PATH", "") | |
viper.SetDefault("LOGGREGATOR_CERT_PATH", "") |
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.
we need that, otherwise you cannot set them up properly without providing a config 🤷♂️
Co-Authored-By: Mark Yen <[email protected]>
Fixes #13
With this change, now we don't fail anymore if a config file is not provided:
With a config file, we keep reading it correctly:
while we allow now to setup configuration via environment variables and also override the config file:
Note:
EIRINI_LOGGREGATOR_BRIDGE_LOGLEVEL=DEBUG
must be set to show the config values that are used