Adds Java version 17 in Github CI build #1609
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1609 +/- ##
============================================
+ Coverage 64.57% 64.60% +0.03%
- Complexity 3215 3216 +1
============================================
Files 247 247
Lines 17351 17352 +1
Branches 3082 3082
============================================
+ Hits 11205 11211 +6
+ Misses 4597 4594 -3
+ Partials 1549 1547 -2
Continue to review full report at Codecov.
|
We do not mention it specifically anywhere in the plugin code as far as java versions go....as it is inferred from OpenSearch's documentation... But perhaps we should? |
DarshitChanpura
dismissed stale reviews from peternied and davidlago
via
0e8c87b
|
Ok, let's keep working throuhg these failures as those are the breaking ones for upgrading to 17 once we support it in 2.0. Next up is https://bugs.openjdk.java.net/browse/JDK-8251547, it seems like some of the elliptic curves we support for JWT authentication are no longer supported in JDK17:
@dblock is this something we should explicitly document (for example, add to 2.0 list of breaking changes in release notes etc) or is it assumed that if something does not work on JDK17, then it doesn't work with OpenSearch using JDK17? My gut says err on the side of documenting these things. |
|
Yes, please open documentation issues or make changes as part of the PR. |
|
Actually it looks like we're not on the same page re:1.3.0. We are trying to replace version 14 with an LTS version 11 for builds, and are testing the complete distribution with the bundled version 17. We are bundling 17 in 1.3.0! While users can use any JDK from the list, does this introduce a backwards incompatible change to users in the complete distribution or the -min distribution? Will someone doing, for example, a rolling upgrade from 1.2.4, run into a problem? |
|
Trying to figure out the exception from the CI run for Java 17 build. Exception call stack from Base64Helper$DescriptorNameSetter |
|
Update: Stacktrace from error caused by initialization in Base64Helper$DescriptorNameSetter.getField method |
|
I think to figure out what we need to work around or fix, we should print out the class names and field names that are attempting to be modified, I'm not sure More background on JDK visibility changes over time:
From https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html |
DarshitChanpura
force-pushed
the
add-java17-to-ci-build
branch
from
0e8c87b to
7e60ff1
Compare
| @@ -42,6 +43,7 @@ | |||
| import org.opensearch.security.util.FakeRestRequest; | |||
| import com.google.common.io.BaseEncoding; | |||
|
|
|||
| @Ignore | |||
There was a problem hiding this comment.
Please include a string value to provide context as these tests need to be skipped. This seems like a major functionality regression - why do all the tests need to be blocked instead of some?
| @Ignore | |
| @Ignore("reasoning...") |
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
DarshitChanpura
force-pushed
the
add-java17-to-ci-build
branch
from
0ca769c to
d202106
Compare
… reverts changes made to use junit 5 Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
| @@ -50,6 +53,15 @@ | |||
| new SecureRandom().nextBytes(secretKey); | |||
| } | |||
|
|
|||
| /* | |||
| This test fails during Java 17 build due to a known bug: https://bugs.openjdk.java.net/browse/JDK-8251547 | |||
| TODO: This method should be removed once a fix is implemented | |||
There was a problem hiding this comment.
Rather than create a TODO can you create an issue that captures what is wrong and what should be done (If you know)?
There was a problem hiding this comment.
I already created an issue and connected it to this PR as well
* Adds Java version 17 in Github CI build * Disables HTTPJwtAuthenticatorTest for Java 17 build using junit 4 and reverts changes made to use junit 5 Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Description
Adds support for Java 17 in Github CI build.
Issues Resolved
#1502
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.
Update:
There is a known Java bug mentioned in #1620 which should be addressed in a separate PR.