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

Support specifying repo config via env #363

Merged
merged 2 commits into from Jul 19, 2022
Merged

Support specifying repo config via env #363

merged 2 commits into from Jul 19, 2022

Conversation

vkamra
Copy link
Contributor

@vkamra vkamra commented Jul 19, 2022

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

@vkamra vkamra temporarily deployed to Testing July 19, 2022 17:47 Inactive
@vkamra
Copy link
Contributor Author

vkamra commented Jul 19, 2022

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

@vkamra
Copy link
Contributor Author

vkamra commented Jul 19, 2022

@aartij17 @ryanfkeepers - manual test below:

$ ls ~/.corso.toml
ls: /Users/vaibhavkamra/.corso.toml: No such file or directory

~/work/alcion/corso/src (repo_env_variables) $ BUCKET=XXXXX PREFIX="" ./corso backup create exchange --user XXXX
...
2022-07-19T11:11:03.449-0700	DPANIC	status object count does not match errors + successes	{"objects": 109, "successes": 0, "errors": 0}
2022-07-19T11:11:03.449-0700	DEBUG	Action: Backup performed on 0 of 109 objects within 4 directories.

@ryanfkeepers
Copy link
Contributor

0 of 109 collections doesn't look too successful. But it did run, so that's good.

@vkamra
Copy link
Contributor Author

vkamra commented Jul 19, 2022

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.

@vkamra vkamra temporarily deployed to Testing July 19, 2022 19:21 Inactive
@wlan0 wlan0 temporarily deployed to Testing July 19, 2022 23:06 Inactive
@wlan0 wlan0 temporarily deployed to Testing July 19, 2022 23:06 Inactive
@vkamra vkamra merged commit 9f8287d into main Jul 19, 2022
@vkamra vkamra deleted the repo_env_variables branch July 19, 2022 23:42
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.

None yet

3 participants