👻 Adding ability for the build to use a podman cache #176
Conversation
WalkthroughThe Dockerfile's addon-build stage now uses inline per-command Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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.
Dockerfile
Outdated
| COPY ./ /app/ | ||
| RUN export JAVA_HOME=/usr/lib/jvm/java-17-openjdk | ||
| RUN JAVA_HOME=/usr/lib/jvm/java-17-openjdk mvn clean install -DskipTests=true | ||
| 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 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cache mount uid and assess Maven goal change.
The cache mount specifies uid=1001, but RUN commands execute as root (uid 0) by default. This uid mismatch may cause permission issues or prevent effective caching. Verify that the uid is intentional and correct.
Additionally, the change from mvn clean install to mvn clean package is a functional change: install writes artifacts to the local repository, while package only builds the jar. This optimization works if downstream stages only need the packaged jar (which line 52 confirms), but verify there are no other consumers expecting artifacts in /root/.m2/repository.
To verify the cache mount configuration, please confirm:
- Is the Maven process expected to run as uid=1001, or should this be uid=0 for the root user?
- Is there any dependency on artifacts being installed to the local Maven repository after the build?
🏁 Script executed:
#!/bin/bash
# Description: Verify Maven process user and check for any references to Maven repository artifacts
# Check if there are any references to the .m2 repository in the build process
rg -n "\.m2/repository" --type dockerfile
# Check for any USER directives in addon-build stage
rg -n "FROM registry.access.redhat.com/ubi9/ubi AS addon-build" -A 20 | grep -i "USER"Length of output: 34
Let me adjust the search strategy to examine the Dockerfile more broadly.
Let me adjust the search strategy to examine the Dockerfile without relying on explicit file type detection.
🏁 Script executed:
#!/bin/bash
# Description: Examine Dockerfile structure and verify cache/Maven configuration
# Find and display the entire Dockerfile
echo "=== Full Dockerfile content ==="
fd -e Dockerfile -x cat
echo -e "\n=== Searching for .m2/repository references ==="
fd -e Dockerfile -x rg "\.m2/repository" -n
echo -e "\n=== Searching for USER directives ==="
fd -e Dockerfile -x rg "USER" -n -C 2
echo -e "\n=== Examining lines around cache mount and COPY (lines 20-30 and 50-55) ==="
fd -e Dockerfile -x sed -n '20,30p;50,55p'Length of output: 196
🏁 Script executed:
#!/bin/bash
# Description: Examine Dockerfile structure and verify cache/Maven configuration
# Find and display the entire Dockerfile
echo "=== Full Dockerfile content ==="
fd -name Dockerfile -x cat
echo -e "\n=== Searching for .m2/repository references ==="
fd -name Dockerfile -x rg "\.m2/repository" -n
echo -e "\n=== Searching for USER directives ==="
fd -name Dockerfile -x rg "USER" -n -C 2
echo -e "\n=== Examining specific lines around cache mount and COPY ==="
fd -name Dockerfile -x sed -n '20,30p;50,55p' | cat -nLength of output: 1105
Let me simplify the approach and use more basic commands.
🏁 Script executed:
#!/bin/bash
# Description: Examine Dockerfile with simpler syntax
echo "=== Locating Dockerfile ==="
find . -name "Dockerfile" -type f
echo -e "\n=== Full Dockerfile content ==="
find . -name "Dockerfile" -type f -exec cat {} \;
echo -e "\n=== Searching for .m2/repository references ==="
find . -name "Dockerfile" -type f -exec grep -n "\.m2/repository" {} +
echo -e "\n=== Searching for USER directives ==="
find . -name "Dockerfile" -type f -exec grep -n "USER" {} +
echo -e "\n=== Lines 20-55 from Dockerfile for context ==="
find . -name "Dockerfile" -type f -exec sed -n '20,55p' {} \;Length of output: 6761
Fix cache mount uid mismatch—RUN commands execute as root, not uid 1001.
The cache mount specifies uid=1001, but the addon-build stage runs as root (uid 0) by default—no USER directive is set. This mismatch will prevent effective caching or cause permission issues. Change the cache mount to uid=0 to match the process user:
RUN --mount=type=cache,id=m2_repo,uid=0,target=/root/.m2 JAVA_HOME=/usr/lib/jvm/java-17-openjdk mvn clean package -DskipTests=trueThe Maven goal change from install to package is correct—the subsequent COPY confirms only the jar artifact from target/ is needed, not repository artifacts.
🤖 Prompt for AI Agents
In Dockerfile around line 26, the cache mount uses uid=1001 but the RUN executes
as root (uid 0), causing permission/cache issues; change the mount's uid to 0
(or make the stage run as uid 1001) so the cache ownership matches the process
user, e.g., set uid=0 on the --mount or add a USER directive to run the stage as
1001, and keep the Maven goal as package since only the jar artifact is copied
later.
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. |
* This will save alot of time downloading maven artifacts Signed-off-by: Shawn Hurley <[email protected]>
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
| COPY ./ /app/ | ||
| ENV JAVA_HOME /usr/lib/jvm/java-17-openjdk | ||
| RUN mvn clean install -DskipTests=true | ||
| RUN export JAVA_HOME=/usr/lib/jvm/java-17-openjdk |
There was a problem hiding this comment.
Remove redundant RUN export—not inherited by next layer.
Line 25's RUN export JAVA_HOME=... is a no-op: RUN instructions create separate layers, so environment exports don't persist to line 26. Line 26 already sets JAVA_HOME inline, making line 25 superfluous.
Remove line 25:
- 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 install -DskipTests=true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 install -DskipTests=true |
🤖 Prompt for AI Agents
In Dockerfile around line 25, the RUN export
JAVA_HOME=/usr/lib/jvm/java-17-openjdk is redundant because environment
variables set in a RUN layer do not persist to subsequent layers and line 26
already sets JAVA_HOME inline; remove the entire line 25 to eliminate the no-op
export and rely on the existing ENV or inline setting on line 26.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.