-
Notifications
You must be signed in to change notification settings - Fork 967
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 a config.strategy to enable overwrite of properties from env vars #620
Changes from 7 commits
d9d1f0a
5c04377
30b6556
7fe4788
e302b9a
c14bb77
4571c79
87dc17d
3a9a1d8
90e2465
4937ee4
fd0e418
4fad113
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE module PUBLIC | ||
"-//Puppy Crawl//DTD Check Configuration 1.3//EN" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shell we update/fix the 'Puppy Crawl' comment as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> | ||
"https://checkstyle.org/dtds/configuration_1_3.dtd"> | ||
|
||
<module name="Checker"> | ||
<module name="SuppressionFilter"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE suppressions PUBLIC "-//Puppy Crawl//DTD Suppressions 1.1//EN" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here still revers to Puppy Crawl as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd"> | ||
"https://checkstyle.org/dtds/suppressions_1_1.dtd"> | ||
<suppressions> | ||
<!-- don't care about javadoc coverage of methods in impl --> | ||
<suppress checks="JavadocMethod" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ | |
*/ | ||
public final class ConfigFactory { | ||
private static final String STRATEGY_PROPERTY_NAME = "config.strategy"; | ||
private static final String OVERRIDE_WITH_ENV_PROPERTY_NAME = "config.override_with_env_vars"; | ||
|
||
private ConfigFactory() { | ||
} | ||
|
@@ -383,7 +384,11 @@ public static Config defaultReference(ClassLoader loader) { | |
* @return the default override configuration | ||
*/ | ||
public static Config defaultOverrides() { | ||
return systemProperties(); | ||
if (getOverrideWithEnv()) { | ||
return systemEnvironmentOverrides().withFallback(systemProperties()); | ||
} else { | ||
return systemProperties(); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -394,7 +399,7 @@ public static Config defaultOverrides() { | |
* @return the default override configuration | ||
*/ | ||
public static Config defaultOverrides(ClassLoader loader) { | ||
return systemProperties(); | ||
return defaultOverrides(); | ||
} | ||
|
||
/** | ||
|
@@ -549,6 +554,50 @@ public static Config systemProperties() { | |
return ConfigImpl.systemPropertiesAsConfig(); | ||
} | ||
|
||
/** | ||
* Gets a <code>Config</code> containing the system's environment variables | ||
* used to override configuration keys. | ||
* Environment variables taken in considerations are starting with | ||
* {@code CONFIG_FORCE_} | ||
* | ||
* <p> | ||
* Environment variables are mangled in the following way after stripping the prefix "CONFIG_FORCE_": | ||
* <table border="1"> | ||
* <tr> | ||
* <th bgcolor="silver">Env Var</th> | ||
* <th bgcolor="silver">Config</th> | ||
* </tr> | ||
* <tr> | ||
* <td>_ [1 underscore]</td> | ||
* <td>. [dot]</td> | ||
* </tr> | ||
* <tr> | ||
* <td>__ [2 underscore]</td> | ||
* <td>- [dash]</td> | ||
* </tr> | ||
* <tr> | ||
* <td>___ [3 underscore]</td> | ||
* <td>_ [underscore]</td> | ||
* </tr> | ||
* </table> | ||
* | ||
* <p> | ||
* A variable like: {@code CONFIG_FORCE_a_b__c___d} | ||
* is translated to a config key: {@code a.b-c_d} | ||
* | ||
* <p> | ||
* This method can return a global immutable singleton, so it's preferred | ||
* over parsing system properties yourself. | ||
* <p> | ||
* {@link #defaultOverrides} will include the system environment variables as | ||
* overrides if `config.override_with_env_vars` is set to `true`. | ||
* | ||
* @return system environment variable overrides parsed into a <code>Config</code> | ||
*/ | ||
public static Config systemEnvironmentOverrides() { | ||
return ConfigImpl.envVariablesOverridesAsConfig(); | ||
} | ||
|
||
/** | ||
* Gets a <code>Config</code> containing the system's environment variables. | ||
* This method can return a global immutable singleton. | ||
|
@@ -1063,4 +1112,10 @@ private static ConfigLoadingStrategy getConfigLoadingStrategy() { | |
return new DefaultConfigLoadingStrategy(); | ||
} | ||
} | ||
|
||
private static Boolean getOverrideWithEnv() { | ||
String overrideWithEnv = System.getProperties().getProperty(OVERRIDE_WITH_ENV_PROPERTY_NAME); | ||
|
||
return Boolean.parseBoolean(overrideWithEnv); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the value of the property is not a boolean this will throw an ugly and likely-mysterious exception right? might want to catch it and say something like "the value of OVERRIDE_WITH_ENV_PROPERTY_NAME must be a boolean not '%s'" Not sure if we already have this bug when looking at any other env var values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've deliberately choose to use Boolean.parseBoolean because it doesn't throw exceptions. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -32,6 +32,7 @@ | |||
* For use only by the {@link com.typesafe.config} package. | ||||
*/ | ||||
public class ConfigImpl { | ||||
private static final String ENV_VAR_OVERRIDE_PREFIX = "CONFIG_FORCE_"; | ||||
|
||||
private static class LoaderCache { | ||||
private Config currentSystemProperties; | ||||
|
@@ -360,6 +361,57 @@ public static void reloadEnvVariablesConfig() { | |||
EnvVariablesHolder.envVariables = loadEnvVariables(); | ||||
} | ||||
|
||||
private static AbstractConfigObject loadEnvVariablesOverrides() { | ||||
Map<String, String> env = new HashMap(System.getenv()); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mean to bike shed -- especially as I've seen you've written tests against this. I just though I'd add a suggestion which could (1) perhaps make the different override cases more easily scannable/readable and (2) make the test cases easier: Instead of conflating the 'System.getenv()' call within the same method which does the override logic, you might consider having something like: static String envVariableAsProperty(envVariable : String, envVarOverridePrefix : String) : String = { which you can then just test via: assertEquals(envVariableAsProperty("PREFIX_A_B__C", "PREFIX_"), "a.b-c") etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it makes totally sense, thanks for pushing me out from my laziness :-) |
||||
Map<String, String> result = new HashMap(System.getenv()); | ||||
for (String key : env.keySet()) { | ||||
if (key.startsWith(ENV_VAR_OVERRIDE_PREFIX)) { | ||||
StringBuilder builder = new StringBuilder(); | ||||
|
||||
String strippedPrefix = key.substring(ENV_VAR_OVERRIDE_PREFIX.length(), key.length()); | ||||
|
||||
int underscores = 0; | ||||
for (char c : strippedPrefix.toCharArray()) { | ||||
if (c == '_') { | ||||
underscores++; | ||||
} else { | ||||
switch (underscores) { | ||||
case 1: builder.append('.'); | ||||
break; | ||||
case 2: builder.append('-'); | ||||
break; | ||||
case 3: builder.append('_'); | ||||
break; | ||||
} | ||||
underscores = 0; | ||||
builder.append(c); | ||||
} | ||||
} | ||||
|
||||
String propertyKey = builder.toString(); | ||||
result.put(propertyKey, env.get(key)); | ||||
} | ||||
} | ||||
|
||||
return PropertiesParser.fromStringMap(newSimpleOrigin("env variables overrides"), result); | ||||
} | ||||
|
||||
private static class EnvVariablesOverridesHolder { | ||||
static volatile AbstractConfigObject envVariables = loadEnvVariablesOverrides(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we need to make ConfigFactory.invalidateCaches clear this
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||
} | ||||
|
||||
static AbstractConfigObject envVariablesOverridesAsConfigObject() { | ||||
try { | ||||
return EnvVariablesOverridesHolder.envVariables; | ||||
} catch (ExceptionInInitializerError e) { | ||||
throw ConfigImplUtil.extractInitializerError(e); | ||||
} | ||||
} | ||||
|
||||
public static Config envVariablesOverridesAsConfig() { | ||||
return envVariablesOverridesAsConfigObject().toConfig(); | ||||
} | ||||
|
||||
public static Config defaultReference(final ClassLoader loader) { | ||||
return computeCachedConfig(loader, "defaultReference", new Callable<Config>() { | ||||
@Override | ||||
|
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'd probably leave this section out, if people really care they can look in the code or commit messages (maybe it's worth moving it to one of those places)
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.
moved to a comment in the code