vbhanuchander-lang opened a new pull request, #8638:
URL: https://github.com/apache/hop/pull/8638
Closes #8495.
`Variables.initializeFrom` seeds the variable space from
`System.getProperties()` and from the described variables in `hop-config.json`,
but never from `System.getenv()`:
```java
Set<String> systemPropertiesNames =
System.getProperties().stringPropertyNames();
for (String key : systemPropertiesNames) {
getProperties().put(key, System.getProperties().getProperty(key));
}
```
So an exported environment variable is not resolvable, and a project *Home
folder* of `${PROJECT_HOME_PATH}` stays literal — even though
`ProjectConfig.getActualProjectConfigFilename` does call
`variables.resolve(getProjectHome())`. The resolution was never the problem;
the name simply was not in the variable space.
## Change
`HOP_IMPORT_ENVIRONMENT_VARIABLES`, **off by default**, makes the operating
system environment part of the variable space. It is read through
`HopResolvedSettings`, so the flag itself can be set from the environment —
which is what a container deployment has to hand, and what @hansva described
wanting for Docker.
**Off by default**, because the whole environment becomes visible as
variables when it is on: `PATH`, `HOME`, credentials injected by a platform,
everything. That is the bleed-over @mattcasters raised, and it seemed better to
make enabling it a deliberate act than to widen the variable space for every
existing installation.
**The environment is seeded before everything else, so it has the lowest
precedence.** A name that is also set with `-D`, in `hop-config.json`, by a
parent variable space, or by injection keeps the value it resolves to today,
and the environment only supplies names that nothing else defines. No existing
configuration changes behaviour, with the flag on or off.
That ordering is not arbitrary: `HopResolvedSettings` already resolves
**hop-config → environment → `-D`**, and its javadoc notes that the variable
space is not yet aligned with that. This is a step towards it that changes
nothing which currently works.
@mattcasters — your sketch was
`System.getenv().forEach(System::setProperty)`. I went with seeding the
variable space directly instead of copying into system properties, because
`System.setProperty` would make the environment win over `hop-config.json`
(described variables are applied after system properties in `initializeFrom`,
so they would be overwritten) and would leak into every other
`System.getProperty` caller in the JVM. Happy to switch if you would rather
have the simpler form.
## Tests
`VariablesTest`, three cases, picking an environment entry that is not also
a system property so what it resolves to is unambiguous:
- `environmentIsNotImportedByDefault` — the environment stays out unless
asked for
- `environmentIsImportedWhenEnabled` — `${NAME}` resolves an exported
variable. **This one fails without the change**, with `expected:
</opt/homebrew> but was: <null>`
- `systemPropertiesWinOverTheEnvironment` — `-D` still wins, pinning the
precedence
`VariablesTest` is 9/9, and `spotless:check` passes (verified the goal
actually executed, 0 files needing changes).
## Open question
Two things I deliberately did not decide:
1. **The default.** Off means the reported symptom still needs one flag set
before `${PROJECT_HOME_PATH}` works. If you would rather it worked out of the
box, the flag can default to `Y` — it is a one-character change and the
precedence already guarantees nothing existing is overridden.
2. **Filtering instead of a flag.** An alternative to a boolean is to import
only names matching a prefix (`HOP_*`, or a configurable one). That avoids the
bleed-over entirely and could then be on by default, at the cost of not
resolving arbitrary names like `PROJECT_HOME_PATH` — which is what the issue
actually asks for. I did not do this, but say the word if you prefer it.
--
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]
