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

Implement idiomatic environment variable handling #414

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rocketraman
Copy link
Contributor

@rocketraman rocketraman commented Mar 29, 2024

Leverage the capability exposed by #413 to implement idiomatic environment variable handling.

Fixes #410.

I've tried to implement this in a backward compatible way, but if we are ok with breaking compatibility for 3.0, given the new path normalization capability, we could replace the existing logic for env vars and simplify it down to always apply env vars with the new logic, removing the EnvironmentVariableOverridePropertySource and simplifying EnvironmentVariablesPropertySource to remove all the config vals, which is what I understood from #413 (comment).

This PR is stacked on top of #413 and will be rebased and moved out of draft once that PR is merged.

There is a test failure in this PR which will be resolved once #444 is merged.

@rocketraman rocketraman force-pushed the rg/env-var-idiomatic branch 2 times, most recently from fafb6a4 to 45e0f94 Compare April 1, 2024 13:48
@rocketraman rocketraman force-pushed the rg/env-var-idiomatic branch 3 times, most recently from 5448e02 to 7bcd7ab Compare April 4, 2024 03:40
@rocketraman rocketraman mentioned this pull request Apr 4, 2024
@rocketraman rocketraman force-pushed the rg/env-var-idiomatic branch 4 times, most recently from f0f6b50 to 29ff9c4 Compare April 4, 2024 17:57
@rocketraman rocketraman force-pushed the rg/env-var-idiomatic branch from 29ff9c4 to e5b0a86 Compare July 15, 2024 16:10
@rocketraman rocketraman marked this pull request as ready for review July 15, 2024 16:11
@rocketraman
Copy link
Contributor Author

@sksamuel Now that #413 is merged (thank you!), I have rebased this.

@rocketraman
Copy link
Contributor Author

@sksamuel Any update on this?

@sksamuel
Copy link
Owner

Happy for this to be breaking now, and we'll cut 3.0

@rocketraman
Copy link
Contributor Author

rocketraman commented Nov 28, 2024

@sksamuel Ok, then here is what I propose (this is what would be in the updated README)... please review and let me know if you are ok with this, and if you are I will make the appropriate changes.


EnvironmentVariablesPropertySource

The EnvironmentVariablesPropertySource reads config from environment variables. This property source maps environment variable names to config properties via idiomatic conventions for environment variables. Env vars are idiomatically UPPERCASE and contain only letters (A to Z), digits (0 to 9), and the underscore (_) character.

Hoplite maps env vars as follows:

  • Underscores are separators for nested config. For example TOPIC_NAME would override a property name located in a topic parent.

  • To bind env vars to arrays or lists, postfix with an index e.g. set env vars TOPIC_NAME_0 and TOPIC_NAME_1 to set two values for the name list property.

  • To bind env vars to maps, the key is part of the nested config e.g. TOPIC_NAME_FOO and TOPIC_NAME_BAR would set the "foo" and "bar" keys for the name map property. Note that keys are one exception to the idiomatic uppercase rule -- the env var name determines the case of the map key.

If the optional (not specified by default) prefix setting is provided, then only env vars that begin with the prefix are considered, and the prefix is stripped from the env var before processing.

As of Hoplite 3, the EnvironmentVariablesPropertySource is applied by default and may be used to override other config properties directly. There is no longer any built-in support for the config.override. prefix. However, the optional prefix setting can still be used for the same purpose.

Single underscores as path separators, uppercase.

Always loaded by default in place of the override source.

The override source is now unnecessary and removed.

Improved path normalization disambiguation, both for classes with
case-ambiguous properties and maps.
@rocketraman
Copy link
Contributor Author

rocketraman commented Nov 28, 2024

@sksamuel I've rebased this branch on the latest master, and in line with my previous message I've pushed the changes to implement the breaking but simpler (i.e. less configurable, more idiomatic) but overall more capable EnvironmentVariablesPropertySource.

Most of the changes to unrelated tests were because the defaults have changed, and now include the default environment.

I've also added support for Spring-like env var to array/list configuration, which should resolve #402.

@rocketraman
Copy link
Contributor Author

@sksamuel bump

@sksamuel
Copy link
Owner

sksamuel commented Feb 2, 2025

Looks good, I guess if you want to use a prefix, you have to remove all the default sources, and add them all back in, but with a custom EnvironmentVariablesPropertySource.
Maybe we should just not have env registered by default anymore?

@rocketraman
Copy link
Contributor Author

Looks good, I guess if you want to use a prefix, you have to remove all the default sources, and add them all back in, but with a custom EnvironmentVariablesPropertySource.

@sksamuel My thinking was that if people want to customize property sources, including the EnvironmentVariablesPropertySource then the normal pattern would be to initialize HopLite via ConfigLoaderBuilder.defaultWithoutPropertySources(), and then add in the customized property sources manually.

There is also the ConfigLoaderBuilder.emptyByDefaultPropertySources() which gets the user all the defaults except the EnvironmentVariablesPropertySource, so users could do something like:

ConfigLoaderBuilder
  .defaultWithoutPropertySources()
  .addPropertySource(EnvironmentVariablesPropertySource(prefix = myPrefix))
  .emptyByDefaultPropertySources()

We could potentially make the pattern simpler with some more public funs on ConfigLoaderBuilder, maybe something like defaultWithEnvVarPrefix(prefix: String) and defaultPropertySourcesWithEnvVarPrefix(prefix: String).

Maybe we should just not have env registered by default anymore?

That is an option as well. I do think with the popularity of 12-factor apps, env vars are now very commonly used for config overrides (and rightly so), so I would hesitate to remove it from the defaults.

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.

Support idiomatic environment variable naming
2 participants