gemini-code-assist[bot] commented on code in PR #39402:
URL: https://github.com/apache/beam/pull/39402#discussion_r3622105639
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -2456,6 +2456,15 @@ class BeamModulePlugin implements Plugin<Project> {
return tags
}
+ // Registry ref for Docker BuildKit layer cache (tag is always buildcache).
+ project.ext.containerRegistryBuildCacheRef = { String imageName ->
+ int lastColon = imageName.lastIndexOf(':')
+ if (lastColon < 0) {
+ return "${imageName}:buildcache"
+ }
+ return "${imageName.substring(0, lastColon)}:buildcache"
+ }
Review Comment:
Using `imageName.lastIndexOf(':')` to find the tag separator is fragile when
the image name contains a registry with a port number (e.g.,
`localhost:5000/beam_python3.9_sdk`). In such cases, the last colon might be
the port separator rather than a tag separator, leading to incorrect image
names like `localhost:buildcache`.
Additionally, we should defensively check if `imageName` is null or empty to
avoid a `NullPointerException`.
We can make this robust by checking if the last colon appears after the last
slash.
```
project.ext.containerRegistryBuildCacheRef = { String imageName ->
if (!imageName) {
return null
}
int lastSlash = imageName.lastIndexOf('/')
int lastColon = imageName.lastIndexOf(':')
if (lastColon < 0 || lastColon < lastSlash) {
return "${imageName}:buildcache"
}
return "${imageName.substring(0, lastColon)}:buildcache"
}
```
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamDockerPlugin.groovy:
##########
@@ -278,6 +288,14 @@ class BeamDockerPlugin implements Plugin<Project> {
if (ext.pull) {
buildCommandLine.add '--pull'
}
+ if (ext.buildx && !ext.cacheFromRefs.isEmpty()) {
+ for (String cacheRef : ext.cacheFromRefs) {
+ buildCommandLine.addAll('--cache-from',
"type=registry,ref=${cacheRef}" as String)
+ }
+ }
+ if (ext.buildx && ext.cacheToRef != null) {
+ buildCommandLine.addAll('--cache-to',
"type=registry,ref=${ext.cacheToRef},mode=max" as String)
+ }
Review Comment:
In Groovy/Java, `List.addAll` expects a `Collection` as its argument.
Calling `buildCommandLine.addAll(String, String)` will throw a
`MissingMethodException` at runtime because there is no `addAll(Object...)`
method on standard lists.
To fix this, wrap the arguments in a list (e.g., `['--cache-from', ...]`) so
that it is passed as a single collection argument.
```
if (ext.buildx && !ext.cacheFromRefs.isEmpty()) {
for (String cacheRef : ext.cacheFromRefs) {
buildCommandLine.addAll(['--cache-from',
"type=registry,ref=${cacheRef}" as String])
}
}
if (ext.buildx && ext.cacheToRef != null) {
buildCommandLine.addAll(['--cache-to',
"type=registry,ref=${ext.cacheToRef},mode=max" as String])
}
```
--
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]
