-
Notifications
You must be signed in to change notification settings - Fork 27
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
Support specifying repo config via env #363
Conversation
Still testing - will update tests also. |
@@ -20,6 +20,14 @@ var ( | |||
errMissingRequired = errors.New("missing required storage configuration") | |||
) | |||
|
|||
// envvar consts | |||
// TODO: Remove these and leverage Viper AutomaticEnv() instead |
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 could be missing easy answers here, but there are two reasons we're using os.GetEnv and manual fallback instead of viper automation:
- Certain flows (ie: init) are expected to not read from a config file, but still utilize env/flag fallbacks.
- In a situation where a config file is expected, but not found, viper did not attempt to parse env values. It seems like a config file must be present in order for viper to mediate fallback options.
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.
The first one (i.e. don't read from config file during init) has made our implementation a bit complicated and I think that's the one to perhaps simplify. Possibly - change the semantics and always allow a config file to be read.
For the latter - I think if we used viper as intended - then some of the env/flag override/fallbacks will fall out of it correctly.
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.
spf13/viper#761 is relevant to our discussion. Indicates why viper's env fallback doesn't work with our current implementation and some things we may need to do to workaround (set default values or bind)
@aartij17 @ryanfkeepers - manual test below:
|
0 of 109 collections doesn't look too successful. But it did run, so that's good. |
Ugh - indeed. I did not notice that and backups are getting created. |
This allows the user to specify repo configuration via environment variables when a config file is not available.
This is a temporary fix - ideally we would leverage Viper for this but we currently only use Viper if a config file
is available so that needs to be refactored a bit.
[1]
TENANT_ID
(already supported)BUCKET
ENDPOINT
PREFIX