Re: [PR] Issue #8495 : Optionally resolve OS environment variables as Hop variables (hop)

["mattcasters (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@hop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org