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

Treat empty environment variables as unset #78

Merged
merged 1 commit into from
Sep 26, 2018
Merged

Treat empty environment variables as unset #78

merged 1 commit into from
Sep 26, 2018

Conversation

philbooth
Copy link
Contributor

Not sure what you'll think of this change, but it comes from a request from our ops folk. Essentially they were expecting an empty environment variable to fall back to the default value rather than overwrite it.

From an abundance of caution it's conditionally compiled behind a feature flag, as I presume it will be a breaking change for some people. Is there any chance of landing this (or something like it)?

@mehcode
Copy link
Owner

mehcode commented Sep 26, 2018

Hm. As I wrote a large chunk of this lib from reading through viper it definitely feels like an oversight on my part as viper ignores empty env vars.

And then we have bash:

❯ FOO= echo ${FOO:-20}
20

❯ echo ${FOO:-20}
20

Technically it's a behavioral breaking change but I'd probably file this under a bug. I'll merge this in a slight variation so that I can remove the config later without a breaking change. You'll want to update to 0.9.1 and do environment.ignore_empty(true) here: https://github.com/mozilla/fxa-email-service/blob/73d07bd7cc89181f759f7cbc99be2687f9b88331/src/settings/mod.rs#L386

If/when we decide to make this the default/only behavior we can softly deprecate that method. Removing a feature flag is a breaking change.

@mehcode mehcode merged commit a1cd868 into mehcode:master Sep 26, 2018
@philbooth
Copy link
Contributor Author

Thanks @mehcode!

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

2 participants