-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Preserve the environment when running cli tools #13689
base: main
Are you sure you want to change the base?
Conversation
❌ Gradle check result for 0eb90c9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Why does PATH matter? Shouldn't |
In the output that I provided a |
I wonder should we pass all the environment? I think so because other environment variables also may affect on script execution. |
Maybe. It also looks like something very non-standard is going on this particular system? Cause it has been working pretty much for everyone, no? |
@@ -782,6 +782,13 @@ private Map<String, String> getOpenSearchEnvironment() { | |||
defaultEnv.put("HOSTNAME", HOSTNAME_OVERRIDE); | |||
defaultEnv.put("COMPUTERNAME", COMPUTERNAME_OVERRIDE); | |||
|
|||
// Pass PATH environment variable to be able to run OpenSearch cli tools |
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 believe we may not be fixing the right place, the shell scripts ./bin/opensearch-plugin
should handle that gracefully? (since this is where the failure happens)
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.
When opensearch-plugin
script is executed it does not have any information about an environment of its parent process as the environment was cleared.
I think we should preserve all the environment like a shell do it for us when we run programs.
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 think we should preserve all the environment like a shell do it for us when we run programs.
Correct, sorry if it was not clear, I meant not messing up with path but check if dirname
is available, and if not use alternative means ?
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.
By default Exec
task keeps the environment if we do not provide ours:
The environment variables to use for the process. Defaults to the environment of this process.
When we call a setEnvironment
method we should merge our environment with the environment of the process (if we need it of course).
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.
Correct, sorry if it was not clear, I meant not messing up with path but check if dirname is available, and if not use alternative means ?
How it should guess where to look for a program? PATH
variable is designed especially for this.
User may have their own standard utility as dirname
somewhere in their home directory, for example inside ~/.bin
. And they want all scripts and programs use it so they add ~/.bin
into a PATH
variable. And that's it. Now all the programs should use their version of dirname
including OpenSearch cli tools.
Yeah. NixOS is a very specific Linux distro. Its But anyway I believe that correct behavior is to rely on a |
It works on other systems because a shell has a default
|
0eb90c9
to
b820d3d
Compare
❌ Gradle check result for b820d3d: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
b820d3d
to
fbcc913
Compare
@@ -742,7 +742,8 @@ private void runOpenSearchBinScript(String tool, CharSequence... args) { | |||
} | |||
|
|||
private Map<String, String> getOpenSearchEnvironment() { | |||
Map<String, String> defaultEnv = new HashMap<>(); | |||
Map<String, String> defaultEnv = System.getenv().entrySet().stream() |
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.
This seems to not go into right direction, see please reasoning here https://github.com/opensearch-project/OpenSearch/pull/13689/files#diff-0eecf581460b128c3d70a559dfdabece54a006048cc0a4e773d020ce1f04fc9bL814
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 see. Then we should keep only a PATH
variable.
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.
Do you agree that we should preserve at least PATH
here?
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.
Are you agree that we should preserve at least
PATH
here?
So any environment injection (as per https://github.com/opensearch-project/OpenSearch/pull/13689/files#diff-0eecf581460b128c3d70a559dfdabece54a006048cc0a4e773d020ce1f04fc9bL814) risks to harm reproducibility, but I don't really know the background of why things were done this way, @dblock do you?
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 do not.
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.
The original comment though is pretty clear. It just wants a clean slate of an environment to make things consistent.
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.
Correct, so we should not pursue the route of injecting env variables @anti-social
❌ Gradle check result for fbcc913: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
What if the change was something more directed, like an actual setting that defines |
We could explicitly set
But to be honest I don't like this solution. Not passing |
I think acceptable solution could be taking into account an |
fbcc913
to
1a92df2
Compare
1a92df2
to
f217270
Compare
❌ Gradle check result for 1a92df2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Some operating systems may have a shell without default PATH variable compiled in, so we should take into account system PATH Signed-off-by: Alexander Koval <kovalidis@gmail.com>
❌ Gradle check result for 3f8e7d4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 3f8e7d4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f217270: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
I think you could set env variables while running the command line:
|
Of course I have |
First, thank you for hanging with us here @anti-social. I have to say, I am very uncomfortable with this change, it has a lot of side effects that can break things for developers on other platforms. Passing PATH, doing source .profile, all that looks like very intrusive workarounds. I'm not sure what's best here, but I would say something that the developer configures explicitly only on NixOS would probably be acceptable. |
❌ Gradle check result for f217270: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
This PR is stalled because it has been open for 30 days with no activity. |
Description
In some operating systems (for example NixOS) there is no default
PATH
variable for a shell (in NixOS they compile it with an option-DDEFAULT_PATH_VALUE="/no-such-path"
). As a result it is not possible to run integration tests. They fail with weird error:The PR solves it explicitly passing
PATH
variable to the environment of CLI tools.Related Issues
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.