mattcasters commented on code in PR #8638:
URL: https://github.com/apache/hop/pull/8638#discussion_r4153822807
##########
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:
**[suggestion]** These tests never exercise the startup order that actually
builds a variable space. `environmentIsNotImportedByDefault` calls
`initializeFrom(null)` against the real process environment, so an exported
`HOP_IMPORT_ENVIRONMENT_VARIABLES=Y` turns the feature on and `assertNull`
fails. The enabled cases only set the system property, which is the one signal
`HopEnvironment` will not overwrite, so they stay green while the environment
path is broken. `environmentEntryNotShadowedBySystemProperty()` also takes
`findFirst()` on an unordered map and ignores hop-config described variables,
which `initializeFrom` applies after the environment and can make the enabled
assertion fail for an unlucky name.
**Suggestion:** `assumeTrue` that the process environment does not already
define the flag, and move the on/off decision into a method that takes the
hop-config value, the environment value, and the system property so the
seeded-`N`-plus-environment-`Y` case can be asserted without mutating
`System.getenv()`.
##########
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:
**[bug]** `HopResolvedSettings.resolveString` cannot see an exported
`HOP_IMPORT_ENVIRONMENT_VARIABLES=Y` once Hop has started. `HopEnvironment`
registers this ENGINE `@Variable` (default `N`) and, when no `-D` is present,
copies that value onto the system properties. `resolveString` applies
hop-config, then the environment, then the system property, so the seeded `N`
wins and `initializeFrom` never imports the environment. `hop-gui` and
`hop-run` both call `HopEnvironment.init()` before
`Variables.getADefaultVariableSpace()`, so the container setup described on
this method does not work. `-D` and a hop-config value of `Y` still work. The
same helper is safe for `HOP_DISABLE_CONSOLE_LOGGING` only because that one is
read in `HopClientEnvironment.init` before the copy.
`LineageVariables.engineVariables` already documents this trap and overlays its
`HOP_LINEAGE_*` environment values after `initializeFrom` for that reason.
**Suggestion:** Do not use `resolveString` for a flag whose own default is
published as a system property. Treat a system property that merely repeats the
hop-config / annotation default as not a `-D` override: a property that differs
from that default wins, otherwise
`System.getenv(Const.HOP_IMPORT_ENVIRONMENT_VARIABLES)` wins, otherwise
hop-config / `N`. Prefer `System.getenv(String)` over `containsKey` on the map
so Windows remains case-insensitive. An explicit
`-DHOP_IMPORT_ENVIRONMENT_VARIABLES=N` is indistinguishable from the seeded
default; call that out. Cover it with a test that passes the three layers in,
because `System.getenv()` cannot be changed from the test.
--
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]
