-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Use bundled JDK in Sys V init #45593
Use bundled JDK in Sys V init #45593
Conversation
This commit addresses an issue when trying to using Elasticsearch on systems with Sys V init and the bundled JDK was not being used. Instead, we were still inadvertently trying to fallback on the path. This commit removes that fallback as that is against our intentions for 7.x where we only support the bundled JDK or an explicit JDK via JAVA_HOME.
Pinging @elastic/es-core-infra |
assumeThat(distribution().hasJdk, is(true)); | ||
|
||
final Result readlink = sh.run("readlink /usr/bin/java"); | ||
assertThat(readlink.exitCode, equalTo(0)); |
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.
Shell.run
already checks that exitCode
is 0.
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.
Thanks, I pushed 4581efe.
echo "Could not find any executable java binary. Please install java in your PATH or set JAVA_HOME" | ||
exit 1 | ||
fi | ||
if [ ! -z "${JAVA_HOME}" ]; then |
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.
We don't seem to actually use JAVA here anywhere. Other than exporting JAVA_HOME, could we remove this logic and let bin/elasticsearch handle it?
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'll explore the consequences of this in a follow-up, I want to get this fix in for 7.3.1.
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.
LGTM
fi | ||
|
||
if [ ! -x "$JAVA" ]; then | ||
echo "Could not find any executable java binary. Please install java in your PATH or set JAVA_HOME" |
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 error message is still referencing PATH
This commit addresses an issue when trying to using Elasticsearch on systems with Sys V init and the bundled JDK was not being used. Instead, we were still inadvertently trying to fallback on the path. This commit removes that fallback as that is against our intentions for 7.x where we only support the bundled JDK or an explicit JDK via JAVA_HOME.
This commit addresses an issue when trying to using Elasticsearch on systems with Sys V init and the bundled JDK was not being used. Instead, we were still inadvertently trying to fallback on the path. This commit removes that fallback as that is against our intentions for 7.x where we only support the bundled JDK or an explicit JDK via JAVA_HOME.
This commit addresses an issue when trying to using Elasticsearch on systems with Sys V init and the bundled JDK was not being used. Instead, we were still inadvertently trying to fallback on the path. This commit removes that fallback as that is against our intentions for 7.x where we only support the bundled JDK or an explicit JDK via JAVA_HOME.
Closes #45542