-
Notifications
You must be signed in to change notification settings - Fork 20
👻 Adding ability for the build to use a podman cache #176
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
base: main
Are you sure you want to change the base?
👻 Adding ability for the build to use a podman cache #176
Conversation
* This will save alot of time downloading maven artifacts Signed-off-by: Shawn Hurley <shawn@hurley.page>
WalkthroughThe Dockerfile's addon-build stage was optimized by replacing Maven's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
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 |
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.
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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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
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.
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. |
Summary by CodeRabbit