Conversation
|
IT tests are in #21987 |
gnodet
left a comment
There was a problem hiding this comment.
LGTM. Clean and straightforward change.
Summary of changes:
- Adds three missing Quarkus Dockerfile variants (
Dockerfile.jvm,Dockerfile.legacy-jar,Dockerfile.native-micro) as resources, aligning with the standard set that Quarkus projects provide. - Updates
ExportQuarkus.copyDockerFiles()to copy all four Dockerfile variants instead of justDockerfile.native. - Bumps the UBI base image comment in
Dockerfile.nativefrom 9.5 to 9.6 (theFROMline already referenced 9.6).
Review notes:
- The
Listimport is already present inExportQuarkus.java, so no new import is needed. - The null check on the
InputStream(if (is != null)) is a good defensive practice. - The Dockerfiles follow the standard Quarkus template patterns (UBI9 base images, proper layering, correct entry points).
- The base class
Export.copyDockerFiles()creates thesrc/main/dockerdirectory and writes a genericDockerfile; the override correctly callssuperfirst, then adds the Quarkus-specific variants. No conflict. - The
Dockerfile.nativecomment fix (9.5 -> 9.6) is a minor correctness improvement.
One minor observation (non-blocking): the PR body is empty. Adding a brief description referencing the JIRA issue would help future readers, but this is cosmetic.
| # accessed directly. (example: "foo.example.com,bar.example.com") | ||
| # | ||
| ### | ||
| FROM registry.access.redhat.com/ubi9/openjdk-21:1.23 |
There was a problem hiding this comment.
Are these Dockerfiles up-to-date?
If you generate an app with the latest Quarkus releases you'll see a base image of registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24.
There was a problem hiding this comment.
Also FWIW - I hacked up a script recently to fetch the latest Dockerfiles. It could perhaps be adapted for this project.
https://github.com/apache/camel-quarkus-examples/blob/camel-quarkus-main/refresh-dockerfiles.sh
There was a problem hiding this comment.
thanks @jamesnetherton I get those version for the current LTS (3.27), do we want latest?
There was a problem hiding this comment.
do we want latest?
I think so because 3.33 is about to become the latest LTS stream.
There was a problem hiding this comment.
done, I kept jdk21 (the latest dockerfile are using jdk25) but I updated it with the latest tag available, thanks!
squakez
left a comment
There was a problem hiding this comment.
I think we remove those resources on purpose. The goal was to harmonize the dockerfiles coming off the export regardless the runtime.
|
The cleaning work was in #18981 |
|
thanks @squakez I didn't know that in the past there were those files, but so I'm wondering why if we generate the Quarkus application using but the jar it uses is not a fat-jar and it doesn't work. So it is a little bit misleading to have this resource |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
yeah the docker files was intented to be like what you get if you create a new quarkus project from code.quarkus.io SB does not come with that. If we can make a docker file for CSB then that is good, if not maybe we then only make these files exported with quarkus. |
|
For the quarkus runtime, isn't best to let the quarkus-container-image-jib extension to take care of it ? It doesn't require a Dockerfile. |
No description provided.