Skip to content
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

Add env var support #134

Closed
wants to merge 5 commits into from
Closed

Conversation

napcae
Copy link

@napcae napcae commented Jul 4, 2019

Addresses #132

@bnfinet
Copy link
Member

bnfinet commented Jul 17, 2019

thanks for starting this effort @napcae

Will you be able to put more effort into this branch?

@napcae
Copy link
Author

napcae commented Jul 17, 2019

Hey @bnfinet, yes I'm willing to but I have a problem understanding how/why the oath key is not read by viper. I asked in IRC but there was no answer thats why there are no new pushes so far.

@napcae napcae marked this pull request as ready for review July 17, 2019 20:14
@bnfinet
Copy link
Member

bnfinet commented Aug 6, 2019

as discussed on IRC we will need to add a defaults.yml file to the cfg.go setup so that viper can seed it's namespace with variable names from defaults.yml. viper will not look for an environmental variable unless it knows the name to look for. viper cannot pull all environmental variables in $VOUCH_*

@napcae
Copy link
Author

napcae commented Sep 3, 2019

Yo @bnfinet, any update?

@bnfinet
Copy link
Member

bnfinet commented Sep 3, 2019

nope, haven't looked at it yet. I'm devoted to another project this week and then I'm off to a conference. Next week looks a bit better.

@bnfinet
Copy link
Member

bnfinet commented Sep 13, 2019

Thanks for the contribution @napcae and sorry for the delay in reviewing.

Regarding this direction from the README:
"you still need to copy the default config.yml since viper needs this variables set before you can override them"

I think that...

It should "just work". If you set an Environmental Variable that conforms to the naming convention, then you're done. This will save much effort for those using Docker Containers since you won't need to map in any config. Even if a config is available and is successfully read I feel that the environmental variable should override.

default.yml should probably by .defaults.yml and it should have a comment at the top that explains that it is for this specific use case, not meant for general consumption. All other comments should be stripped.

.defaults.yml should have values which are consistent with the ones set by cfg.SetDefaults()

cfg.SetDefaults() should be audited and any values which can be replaced by just reading them from .defaults.yml would be preferable.

A log WARN message of some sort should be printed if the config changes (test the config before and after the env load event)

Thanks again and do let me know your thoughts.

@bnfinet
Copy link
Member

bnfinet commented Oct 16, 2019

@napcae I'm going to close this for now.

I'm sure what I suggested is a bit more than you wanted to sign up for with this PR. Of course if that is not the case do let me know and I'd be happy to reopen the PR or continue the discussion in #132.

Thanks again for putting the effort into VP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants