-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Bump to Java11 for GHA runner #32138
Conversation
82284e3
to
b9c00c2
Compare
Ran a couple of representing tests on this branch all passed: https://github.com/apache/beam/actions?query=branch%3Abumptojava11++ including Flink/Spark runner tests that failing on Java17 (#31762) |
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
R: @damccorm |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Found that "-Pjava8Home" property was not effective, running more tests |
Bump default gradle to 8.4 on GHA runner Remove explicit ref to java8 in GHA; bump default GHA java version to 11
SQL PreCommit Java8 ( "verifyCodeIsCompiledWithJava8 0.017s passed " |
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.
Overall LGTM, just had one question.
When we're ready to merge, we should monitor tests closely to make sure it hasn't broken anything, since its tough to test all the GHA workflows.
@@ -70,6 +70,12 @@ public void verifyTestCodeIsCompiledWithJava21() throws IOException { | |||
} | |||
|
|||
// jvm | |||
@Test | |||
public void verifyRunningJVMVersionIs8() { |
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.
Why do we need this one?
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.
And should it replace the Is11 check?
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.
Why do we need this one?
It is needed by the :sdks:java:testing:test-utils:verifyJavaVersion$testVer
target. When gradle command has testJavaVersion
property, this is added as a dependency in a few tests here:
Line 732 in dbd719b
if (project.hasProperty("testJavaVersion")) { |
in order to verify the test indeed running on Java runtime of the version assigned by -PtestJavaVersion=testVer
. Previously GHA test suites only has 11,17,21. Now this PR added a few Java8 tests with -PtestJavaVersion=8
so new test is exercised.
And should it replace the Is11 check?
I'm trying to generalize the buildSrc such that it does not assume the Java platform. Then one can run a test, saying, with default Java being Java8 or 17 and set -PtestJavaVersion=11
. So I added a java8 test entry instead of replace 11 to 8.
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.
Got it, thanks - that makes sense.
Thanks! for sure |
FixAddress #31677 (keep the Issue open for a while to monitor the build status after merge)Bump default gradle to 8.4 on GHA runner
Remove explicit ref to java8 in GHA; bump default GHA java version to 11
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.