Pass COMPUTERNAME env var to elasticsearch.bat #45763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell.
williamrandolph
added
>test
What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it.
rjernst
reviewed
Aug 21, 2019
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing.
alpar-t
reviewed
Aug 21, 2019
buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java
Outdated
Show resolved
Hide resolved
Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
|
@elasticmachine Please run elasticsearch-ci/2 |
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Aug 22, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Aug 22, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Aug 22, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
|
moved release label to 7.3.2 since it just barely missed the 7.3.1 cutoff. |
williamrandolph
added a commit
that referenced
this pull request
Aug 26, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
williamrandolph
added a commit
that referenced
this pull request
Aug 26, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other.
williamrandolph
added a commit
that referenced
this pull request
Aug 26, 2019
* Pass COMPUTERNAME env var to elasticsearch.bat (#45763) * Pass COMPUTERNAME env var to elasticsearch.bat When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that contains the hostname of the machine the script is running on. When there's no provided nodename, Elasticsearch uses the HOSTNAME to create a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for the same purpose. CMD.EXE provides the same thing, except it's called %COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of $COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat, the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null and breaking an integration test on Windows. This commit sets COMPUTERNAME in the environment so that our tests get the value that Elasticsearch would have when bin/elasticsearch.bat is invoked from the shell. * Add null check to protect in non-Windows case What good is it a developer to gain the whole Windows if they forfeit their Unix? The value that fixes things on Windows is null on Linux/Darwin, so let's null-check it. * Override system hostnames for testclusters Rather than relying on variable system behavior, let's just override HOSTNAME and COMPUTERNAME and test for correct values in the integration test that was originally failing. * Rename constants for clarity Since we are setting HOSTNAME and COMPUTERNAME regardless of whether the tests are running on Windows or Linux, we shouldn't imply that constants are only used in one case or the other. * Override hostnames in rest-integ-test clusters too
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we run bin/elasticsearch with bash, we get a $HOSTNAME builtin that
contains the hostname of the machine the script is running on. When
there's no provided nodename, Elasticsearch uses the HOSTNAME to create
a nodename. On Windows, Powershell provides a $COMPUTERNAME variable for
the same purpose. CMD.EXE provides the same thing, except it's called
%COMPUTERNAME%. bin/elasticsearch.bat sets $HOSTNAME to the value of
$COMPUTERNAME. However, when testclusters invokes bin/elasticsearch.bat,
the COMPUTERNAME variable doesn't get passed in, leaving HOSTNAME null
and breaking an integration test on Windows.
This commit sets COMPUTERNAME in the environment so that our tests get
the value that Elasticsearch would have when bin/elasticsearch.bat is
invoked from the shell.
Fixes #44656