vbhanuchander-lang commented on code in PR #8638:
URL: https://github.com/apache/hop/pull/8638#discussion_r4179695926
##########
core/src/main/java/org/apache/hop/core/variables/Variables.java:
##########
@@ -135,6 +145,18 @@ public void initializeFrom(IVariables parent) {
initialized = true;
}
+ /**
+ * Whether the operating system environment should be made available as Hop
variables.
+ *
+ * <p>Resolved through {@link HopResolvedSettings} so the flag itself can be
turned on from the
+ * environment, which is what a container deployment has to hand.
+ */
+ private static boolean isEnvironmentImported() {
+ return "Y"
+ .equalsIgnoreCase(
+
HopResolvedSettings.resolveString(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES, "N"));
Review Comment:
You are right, and it broke exactly the container case the flag was added
for. Fixed in d81db5d836 the way you suggested:
`isEnvironmentImported(configValue, environmentValue, systemPropertyValue)` no
longer goes through `resolveString`. A system property only counts as a `-D`
override when it differs from the hop-config value; otherwise
`System.getenv(name)` wins; otherwise the hop-config value. It compares against
`Const.NVL(configValue, "")`, because that is what `HopEnvironment` writes when
hop-config has the name without a value. It uses `System.getenv(String)`, so
Windows stays case-insensitive. The javadoc calls out that an explicit `-D`
repeating the hop-config value cannot be told apart from the seeded copy.
##########
core/src/test/java/org/apache/hop/core/variables/VariablesTest.java:
##########
@@ -196,4 +201,69 @@ public IHopMetadataProvider getMetadataProvider() {
assertEquals(provider, child.findExecutionMetadataProvider());
assertNull(new Variables().findExecutionMetadataProvider());
}
+
+ /**
+ * An environment entry whose name is not also a system property, so that
what it resolves to is
+ * unambiguous.
+ */
+ private static Map.Entry<String, String>
environmentEntryNotShadowedBySystemProperty() {
+ Set<String> systemPropertyNames =
System.getProperties().stringPropertyNames();
+ return System.getenv().entrySet().stream()
+ .filter(entry -> !systemPropertyNames.contains(entry.getKey()))
+ .filter(entry -> StringUtils.isNotEmpty(entry.getValue()))
+ .findFirst()
+ .orElse(null);
+ }
+
+ @AfterEach
+ void clearEnvironmentImportFlag() {
+ System.clearProperty(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES);
+ }
+
+ /** The operating system environment stays out of the variable space unless
it is asked for. */
+ @Test
+ void environmentIsNotImportedByDefault() {
Review Comment:
All three addressed:
- The decision now takes the three layers as arguments, and is tested
directly. That includes your case (hop-config `N`, seeded system property `N`,
exported `Y` → on), the seeded empty value, the environment overriding
hop-config, and a differing `-D` winning. With the old `resolveString` order
put back, 3 of these fail, including `exportedFlagWinsOverTheSeededDefault`.
- The `initializeFrom` tests `assumeTrue` that the flag is not exported in
the process environment.
- The test entry is now the lowest-sorted environment name that is neither a
system property nor a hop-config described variable, so the choice is
deterministic and `initializeFrom` cannot override it.
`VariablesTest` 15/15, spotless clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
