👻 Adding ability for the build to use a podman cache#176
👻 Adding ability for the build to use a podman cache#176shawn-hurley wants to merge 1 commit intokonveyor:mainfrom
Conversation
📝 WalkthroughWalkthroughThe Dockerfile's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
25-26: Simplify redundant JAVA_HOME setup.Line 25 sets JAVA_HOME as a RUN environment variable, but this only applies to that specific RUN command and is immediately followed by another RUN command that re-sets JAVA_HOME inline on line 26. Consider consolidating these into a single ENV directive to avoid repetition.
Apply this diff to clean up the redundancy:
-RUN export JAVA_HOME=/usr/lib/jvm/java-17-openjdk -RUN --mount=type=cache,id=m2_repo,uid=1001,target=/root/.m2 JAVA_HOME=/usr/lib/jvm/java-17-openjdk mvn clean package -DskipTests=true +ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk +RUN --mount=type=cache,id=m2_repo,uid=1001,target=/root/.m2 mvn clean package -DskipTests=true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
🔇 Additional comments (1)
Dockerfile (1)
52-52: LGTM: COPY path correctly updated forpackagegoal.The COPY instruction now pulls the packaged jar from
/app/java-analyzer-bundle.core/target/, which is the correct output location when usingmvn clean packageinstead ofinstall.
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
I'm not sure this is safe to do in full Konveyor installation by default.
While understanding performance benefits, there could be user roles&credentials for applications setup in Hub where sharing maven cache could expose artifacts between applications, which I'm not sure is wanted.
Keeping it as a comment in case I'm missed something and it is safe to do this.
|
@aufi This only uses the build cache when you are building images in GitHub Actions, and I don't think it has anything to do with the Maven cache when running as a task pod in the hub. Unless I am missing something, this should not be a problem. |
shawn-hurley
force-pushed
the
feature/get-ready-for-composable-build-actions
branch
from
e193492 to
8ba51d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: JDT.LS Integration Tests (Phase 2)
- GitHub Check: Build tackle2-addon-analyzer
🔇 Additional comments (2)
Dockerfile (2)
26-26: Fix critical cache mount uid mismatch—RUN commands execute as root (uid 0), not uid 1001.The
addon-buildstage has no USER directive, so it runs as root (uid 0). The cache mount specifies uid=1001, creating a mismatch that will prevent effective caching or cause permission denied errors when BuildKit attempts to write to the cache with mismatched ownership.Change the cache mount uid to 0 to match the process user:
- RUN --mount=type=cache,id=m2_repo,uid=1001,target=/root/.m2 JAVA_HOME=/usr/lib/jvm/java-17-openjdk mvn clean install -DskipTests=true + RUN --mount=type=cache,id=m2_repo,uid=0,target=/root/.m2 JAVA_HOME=/usr/lib/jvm/java-17-openjdk mvn clean install -DskipTests=trueAdditionally, verify that the Maven goal should be
packageinstead ofinstallif only the jar artifact fromtarget/is needed downstream (lines 54-55).
54-55: Verify Maven build produces the expected artifact paths and versions.The COPY commands on lines 54-55 assume the Maven build produces a jar at
/app/java-analyzer-bundle.core/target/java-analyzer-bundle.core-1.0.0-SNAPSHOT.jar. While thejava-analyzer-bundle.coremodule is confirmed to exist in the project structure, the hardcoded version1.0.0-SNAPSHOTcreates a brittle dependency—if the version inpom.xmldiffers, the build will silently fail to copy the artifact.Confirm that:
- The
java-analyzer-bundle.coremodule'spom.xmlspecifies version1.0.0-SNAPSHOT- Running
mvn clean packagefrom the root produces the jar at exactly this path- If the version is expected to vary or change, consider using a wildcard pattern (e.g.,
java-analyzer-bundle.core-*.jar) or parameterizing the version
shawn-hurley
force-pushed
the
feature/get-ready-for-composable-build-actions
branch
from
8ba51d5 to
86867c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Dockerfile:
- Line 25: The cache mount on the RUN line using
"--mount=type=cache,id=m2_repo,uid=1001,target=/root/.m2" will be removed at the
end of that RUN so artifacts in /root/.m2 won't persist for later COPY
--from=addon-build /root/.m2/repository/...; update the build stage (the RUN
invoking /usr/local/apache-maven-3.9.12/bin/mvn clean install -DskipTests=true)
to, within the same RUN, copy the built artifacts out of the cache directory
into a persistent path inside the build image (e.g., /app/m2-output/) before the
RUN finishes, and remove the uid=1001 option (or set owner to root) to avoid
permission issues; then update the final-stage COPY instructions to pull from
/app/m2-output/ instead of /root/.m2/repository/... so the artifacts are
actually available in the final image.
🧹 Nitpick comments (1)
Dockerfile (1)
28-31: Consider adding error handling for the download URL resolution.If the GitHub API call fails or the grep pattern doesn't match,
DOWNLOAD_URLwill be empty and wget will fail silently (due to--quiet). This could make debugging difficult.Suggested improvement
-RUN DOWNLOAD_URL=$(wget --quiet -O - https://api.github.com/repos/konveyor/maven-search-index/releases/latest | grep '"browser_download_url".*maven-index-data.*\.zip' | sed -E 's/.*"browser_download_url": "([^"]+)".*/\1/') && \ - wget --quiet ${DOWNLOAD_URL} -O maven-index-data.zip && \ +RUN DOWNLOAD_URL=$(wget --quiet -O - https://api.github.com/repos/konveyor/maven-search-index/releases/latest | grep '"browser_download_url".*maven-index-data.*\.zip' | sed -E 's/.*"browser_download_url": "([^"]+)".*/\1/') && \ + [ -n "${DOWNLOAD_URL}" ] || { echo "Failed to resolve maven-index-data download URL"; exit 1; } && \ + wget ${DOWNLOAD_URL} -O maven-index-data.zip && \ unzip maven-index-data.zip && \ rm maven-index-data.zip
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: JDT.LS Integration Tests (Phase 2)
- GitHub Check: image-build / build (arm64)
- GitHub Check: image-build / build (s390x)
- GitHub Check: image-build / build (ppc64le)
- GitHub Check: image-build / build (amd64)
- GitHub Check: Build tackle2-addon-analyzer
* This will save alot of time downloading maven artifacts Signed-off-by: Shawn Hurley <shawn@hurley.page>
shawn-hurley
force-pushed
the
feature/get-ready-for-composable-build-actions
branch
from
86867c4 to
af97f7e
Compare
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.