-
Notifications
You must be signed in to change notification settings - Fork 81
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
update to JDK 21 for integ tests #737
Conversation
@@ -45,7 +45,20 @@ jobs: | |||
with: | |||
distribution: 'adopt' | |||
java-version: '17' | |||
- run: echo "JAVA17_HOME=$JAVA_HOME" >> $GITHUB_ENV | |||
- run: | |
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 should document in a comment in both files why there are two JDK versions being used.
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.
Added some small comments
Approved but will wait before merging because @gkamat wanted check this out |
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 change may fix the integration tests as run via GitHub actions. However, integ tests can be run directly when OSB is installed in development mode, with make it
, and this change will not be applicable in that case. Also, switching the JDK back and forth makes this a bit harder to understand.
The better option would probably be to add JDK 21 to the provision_config
resources:
# major version of the JDK that is used to build OpenSearch
build.jdk = 17
# list of JDK major versions that are used to run OpenSearch
runtime.jdk = 17,16,15,14,13,12,11,8
f321275
to
0c45d20
Compare
Discussed offline: In the meantime, integration tests run with Github Actions will work, but running these tests locally with |
Signed-off-by: Michael Oviedo <[email protected]>
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.
Should be an interim solution to get the GHA integ tests working. Still need to fix the actual integ tests when run via command line.
Description
Integration tests were failing due to recent changes in OpenSearch that require Java 21 for gradle builds:
(reference to the failing tests).
Reference to the OpenSearch PR that updates the JDK version.
To fix this, I added a JDK 21 installation to the integ tests for the gradle builds. However, a JDK version between 11 and 17 is still needed for the integration tests. So, I also modified the
build.sh
file to just use JDK 21 if necessary for the gradle operations, otherwise using JDK 17.I'm open to alternatives if there is a more elegant solution.
Testing
make it
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.