split app into layers for more efficent building - #112
Conversation
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
📝 WalkthroughWalkthroughDockerfile changed build and startup: Maven now extracts runtime dependencies to Changes
Sequence Diagram(s)sequenceDiagram
participant Maven as Build (Maven)
participant Docker as Docker build
participant Image as Runtime image FS
participant JVM as Container JVM
Maven->>Docker: produce `target/deps`, compiled `classes/`
Docker->>Image: COPY dependency jars -> `libs/`
Docker->>Image: COPY compiled classes -> `classes/`
Image->>JVM: container starts
JVM->>Image: run `java -cp "classes:libs/*" org.juv25d.App`
JVM-->>Image: application uses explicit classpath
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Line 31: The Dockerfile currently copies processed resources from the source
path using the COPY instruction "COPY --from=build /app/src/main/resources/
resources/", which bypasses Maven resource filtering; change the COPY to pull
processed classes/resources from Maven's output (e.g., use "COPY --from=build
/app/target/classes/ classes/" or collapse class/resource copies into a single
"COPY --from=build /app/target/classes/ classes/") so the Docker image contains
Maven-filtered/processed resources rather than raw src files.
- Line 11: Update the Dockerfile RUN that executes mvn
dependency:copy-dependencies so it passes -DincludeScope=runtime (leave the rest
of the command intact) to ensure only compile+runtime artifacts are copied into
target/deps and therefore into the libs/ layer of the runtime image; locate the
RUN line invoking mvn dependency:copy-dependencies and add the
-DincludeScope=runtime flag.
- Around line 19-29: The Dockerfile currently copies each org/juv25d subpackage
individually (the multiple COPY --from=build ... org/juv25d/... lines), which
will miss any new packages; replace those explicit per-package COPY lines with a
single recursive copy of the entire Maven output (COPY --from=build
/app/target/classes/ classes/) and remove the now-redundant separate resources
copy and the extra resources classpath entry in the ENTRYPOINT; also update the
ENTRYPOINT to reference only the classes path (drop the former resources entry)
so runtime classpath uses the full target/classes tree.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Dockerfile`:
- Line 31: The Dockerfile copies raw resources from the source tree with the
COPY instruction "COPY --from=build /app/src/main/resources/ resources/", which
bypasses Maven resource filtering; change it to copy processed resources from
the build output (e.g., /app/target/classes/ or the build stage path where Maven
writes processed resources) so that Maven property substitutions and Spring
placeholders are resolved before they are baked into the image; update the COPY
target used in the Dockerfile to reference the processed resources location
instead of /app/src/main/resources/.
- Around line 19-29: The Dockerfile currently enumerates each org/juv25d
subpackage with many COPY lines (e.g., the multiple "COPY --from=build
/app/target/classes/org/juv25d/..." entries) which is fragile and will miss new
packages; replace those per-package COPYs with a single recursive copy of the
entire target/classes directory (copy /app/target/classes/ to classes/), and
remove the separate resources COPY and the corresponding resources entry from
the ENTRYPOINT since Maven already places processed resources into
target/classes; ensure only the single recursive classes copy remains and update
ENTRYPOINT to use the classes classpath entry that matches this change.
|
hmm, does this not mean we have to keep updating the docker file whenever we change our package structure, or add a new package, or remove a package, etc...? Seems like that will be a burden and an easy point of failure? |
|
@fmazmz indeed, after looking into it based on the rabbit feedback it's also redundant. So changing it to only split the dependencies from the classes which should work with changes to our packages. |
… the .jar Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (2)
9-10: Consider combining bothmvninvocations into a singleRUNstep.Two separate Maven JVM startups cost a few seconds each on every build. They can be collapsed into one command without losing anything:
♻️ Proposed refactor
-RUN mvn clean package -DskipTests -Dmaven.shade.skip=true -RUN mvn dependency:copy-dependencies -DoutputDirectory=target/deps -DincludeScope=runtime +RUN mvn -B clean package dependency:copy-dependencies \ + -DskipTests \ + -Dmaven.shade.skip=true \ + -DoutputDirectory=target/deps \ + -DincludeScope=runtimeThis also makes the
-B(batch/no-progress) flag consistent with howgo-offlineis called on line 6.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 9 - 10, Combine the two separate Maven RUN steps into a single RUN to avoid two JVM startups: run mvn with both goals in one invocation (clean package -DskipTests -Dmaven.shade.skip=true dependency:copy-dependencies -DoutputDirectory=target/deps -DincludeScope=runtime) and add the -B flag for batch mode to match the existing go-offline invocation; update the Dockerfile by replacing the two RUN lines referencing "mvn clean package ..." and "mvn dependency:copy-dependencies ..." with a single RUN that includes both goals and -B.
12-12: Prefer explicit Alpine-version tag for consistency with Java 25 security patches.The
eclipse-temurin:25-jre-alpinetag exists and builds successfully. However, unversioned alpine tags (e.g.,25-jre-alpine) can lag behind newer Alpine minor versions in the official-images manifest. To ensure you pull the latest Alpine security patches, use the explicit version-pinned variant:Suggested improvement
-FROM eclipse-temurin:25-jre-alpine +FROM eclipse-temurin:25-jre-alpine-3.23This is a best-practice recommendation, not a blocking issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 12, The FROM line currently uses an unpinned Alpine image tag; update the Dockerfile's FROM directive (the line 'FROM eclipse-temurin:25-jre-alpine') to a version-pinned Alpine variant (for example 'eclipse-temurin:25-jre-alpine3.18' or another current alpine3.x minor) so Java 25 images receive explicit Alpine security patching, then rebuild to verify compatibility and update any Docker documentation if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 9-10: Combine the two separate Maven RUN steps into a single RUN
to avoid two JVM startups: run mvn with both goals in one invocation (clean
package -DskipTests -Dmaven.shade.skip=true dependency:copy-dependencies
-DoutputDirectory=target/deps -DincludeScope=runtime) and add the -B flag for
batch mode to match the existing go-offline invocation; update the Dockerfile by
replacing the two RUN lines referencing "mvn clean package ..." and "mvn
dependency:copy-dependencies ..." with a single RUN that includes both goals and
-B.
- Line 12: The FROM line currently uses an unpinned Alpine image tag; update the
Dockerfile's FROM directive (the line 'FROM eclipse-temurin:25-jre-alpine') to a
version-pinned Alpine variant (for example 'eclipse-temurin:25-jre-alpine3.18'
or another current alpine3.x minor) so Java 25 images receive explicit Alpine
security patching, then rebuild to verify compatibility and update any Docker
documentation if needed.
|
Nice, looks better. |
|
Looks good, I was able to run the docker image fine with no errors. Later we can deploy a new tag to main to get all the new features and deploy a new release! |
Nice, thanks for cleaning up and merging! |
3 participants
Updater dockerfile to handle the different layers instead of entire jar
Summary by CodeRabbit