gemini-code-assist[bot] commented on code in PR #39058:
URL: https://github.com/apache/beam/pull/39058#discussion_r3455034066
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -1628,7 +1631,7 @@ class BeamModulePlugin implements Plugin<Project> {
}
// if specified test java version, modify the compile and runtime
versions accordingly
if (['11', '17', '21', '25'].contains(testJavaVersion)) {
- def testJavaHome = project.getProperty("java${testJavaVersion}Home")
+ def testJavaHome = project.findProperty("java${testJavaVersion}Home")
Review Comment:
If `testJavaVersion` is specified but the corresponding
`java${testJavaVersion}Home` property is not defined, `project.findProperty`
will return `null`. This can lead to a `NullPointerException` or
`IllegalArgumentException` when configuring the compilation and test tasks
later in this block.\n\nWe can make this safer by checking if the property is
defined directly in the `if` condition.
```
if (['11', '17', '21', '25'].contains(testJavaVersion) &&
project.findProperty("java${testJavaVersion}Home")) {\n def testJavaHome
= project.findProperty("java${testJavaVersion}Home")
```
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -2979,10 +2984,11 @@ class BeamModulePlugin implements Plugin<Project> {
]
def serviceArgs =
project.project(':sdks:python').mapToArgString(transformServiceOpts)
def pythonContainerSuffix =
project.project(':sdks:python').pythonVersion.replace('.', '')
- def javaContainerSuffix = getSupportedJavaVersion()
+ String ver = project.findProperty('testJavaVersion')
+ def javaContainerSuffix = ver ? getSupportedJavaVersion(ver) :
getSupportedJavaVersion()
// Transform service delivers transforms that refer to SDK harness
containers with following sufixes.
- def transformServiceJavaContainerSuffix = 'java11'
+ def transformServiceJavaContainerSuffix = 'java17'
Review Comment:
Hardcoding `transformServiceJavaContainerSuffix` to `'java17'` will cause
test failures when running tests on other Java versions (such as Java 11 or
Java 21). This is because the setup task only builds the container image
specified by `javaContainerSuffix` (which is dynamic based on
`testJavaVersion`). If the transform service tries to use `'java17'` but only
`'java11'` was built, the test will fail with an image not found error.\n\nWe
should make `transformServiceJavaContainerSuffix` dynamic by setting it to
`javaContainerSuffix` so that it always matches the container version being
built and tested.
```
def transformServiceJavaContainerSuffix = javaContainerSuffix
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
