-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Introduce a new extension for generating Dockerfiles #42316
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
ad21261
to
1866652
Compare
This comment has been minimized.
This comment has been minimized.
1866652
to
4092aca
Compare
This comment has been minimized.
This comment has been minimized.
4092aca
to
afcb767
Compare
This comment has been minimized.
This comment has been minimized.
afcb767
to
8b617f8
Compare
This comment has been minimized.
This comment has been minimized.
8b617f8
to
7ffcce4
Compare
This comment has been minimized.
This comment has been minimized.
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.
This looks good over all, I spotted only micro issues.
Not sure who would have the time to test this one. @maxandersen do you have somebody in mind?
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-api</artifactId> | ||
<version>1.1.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-impl</artifactId> | ||
<version>1.1.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-spi</artifactId> | ||
<version>1.1.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-connector-basic</artifactId> | ||
<version>1.1.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-transport-file</artifactId> | ||
<version>1.1.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.aether</groupId> | ||
<artifactId>aether-transport-http</artifactId> | ||
<version>1.1.0</version> | ||
<exclusions> | ||
<exclusion> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>*</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> |
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.
The indentation is off here.
static final String DEFAULT_JVM_FROM = "registry.access.redhat.com/ubi8/openjdk-21:1.19"; | ||
static final String DEFAULT_NATIVE_FROM = "registry.access.redhat.com/ubi8/ubi-minimal:8.9"; | ||
|
||
/** | ||
* The from image to use for JVM based Dockerfiles | ||
*/ | ||
@ConfigItem(defaultValue = "registry.access.redhat.com/ubi8/openjdk-21:1.19") | ||
String jvmFrom; | ||
|
||
/** | ||
* The from image to use for native based Dockerfiles | ||
*/ | ||
@ConfigItem(defaultValue = "registry.access.redhat.com/ubi8/ubi-minimal:8.9") | ||
String nativeFrom; |
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.
Not for this PR but we need to centralize all these images reference somewhere. We have them spread all over our code and it's a recipe for disaster.
I will create an issue about this so that we discuss it when @cescoffier is back.
metadata: | ||
keywords: | ||
- "dockerfiles" | ||
guide: "https://quarkus.io/guides/dockerfiles" |
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.
Please only reference a guide here when you have one or @holly-cummins will be unhappy :).
extensions/dockerfiles/cli/pom.xml
Outdated
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-maven-plugin</artifactId> | ||
<executions> | ||
<execution> | ||
<goals> | ||
<goal>build</goal> | ||
<goal>generate-code</goal> | ||
<goal>generate-code-tests</goal> | ||
</goals> | ||
<configuration> | ||
<skipOriginalJarRename>true</skipOriginalJarRename> | ||
<environmentVariables> | ||
<MAVEN_REPO_LOCAL>${settings.localRepository}</MAVEN_REPO_LOCAL> | ||
<GRADLE_OPTS>${env.MAVEN_OPTS}</GRADLE_OPTS> | ||
</environmentVariables> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
<plugin> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<configuration> | ||
<compilerArgs> | ||
<arg>-parameters</arg> | ||
</compilerArgs> | ||
</configuration> | ||
</plugin> | ||
</plugins> | ||
</build> |
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.
Could you indent that part with 4 spaces too.
And yes, we need an XML formatter...
Hey @iocanel . The feature freeze for 3.14 and 3.15 LTS is next Tuesday so we should really try to finalize this as it's an awesome feature. Someone asked about it here #42441 and I was thinking of asking them to test but... we need instructions on how to install it as I don't think it comes by default? |
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 added some additional comments that need fixing.
I saw you asked for a review by @maxandersen and @cescoffier anyway so I will let it there until they have time to review it.
@ConfigItem(defaultValue = "registry.access.redhat.com/ubi8/openjdk-21:1.19") | ||
String jvmFrom; | ||
|
||
/** | ||
* The from image to use for native based Dockerfiles | ||
*/ | ||
@ConfigItem(defaultValue = "registry.access.redhat.com/ubi8/ubi-minimal:8.9") |
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.
BTW, the constants should be used here.
@ConfigRoot | ||
public class DockerfilesConfiguration { | ||
|
||
static final String DEFAULT_JVM_FROM = "registry.access.redhat.com/ubi8/openjdk-21:1.19"; |
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.
static final String DEFAULT_JVM_FROM = "registry.access.redhat.com/ubi8/openjdk-21:1.19"; | |
static final String DEFAULT_JVM_FROM = "registry.access.redhat.com/ubi8/openjdk-21:1.20"; |
public class DockerfilesConfiguration { | ||
|
||
static final String DEFAULT_JVM_FROM = "registry.access.redhat.com/ubi8/openjdk-21:1.19"; | ||
static final String DEFAULT_NATIVE_FROM = "registry.access.redhat.com/ubi8/ubi-minimal:8.9"; |
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.
static final String DEFAULT_NATIVE_FROM = "registry.access.redhat.com/ubi8/ubi-minimal:8.9"; | |
static final String DEFAULT_NATIVE_FROM = "registry.access.redhat.com/ubi8/ubi-minimal:8.10"; |
I'm also wondering if in the future we should generate the default Dockerfiles from there to be consistent. |
@iocanel any reason not to continue on what you started with codestarts? |
It should be possible to share the same templates as the existing codestarts and use a subset part of the codestart generator for generation. |
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 may have missed something but where extensions can add packages and files to the generated docker files?
So far I only see the "from" customization.
What I had in my mind is to deal with just #41772 and leave the rest for future prs. If you feel that this needs to be done in one go, please let me know. |
7ffcce4
to
ab5590e
Compare
This comment has been minimized.
This comment has been minimized.
ab5590e
to
95984d7
Compare
🎊 PR Preview 4cab5df has been successfully built and deployed to https://quarkus-pr-main-42316-preview.surge.sh/version/main/guides/
|
Status for workflow
|
Status for workflow
|
@iocanel Where are we on this? |
@@ -7,6 +7,9 @@ | |||
* This system property is used in rare by libraries (Truffle for example) to create their own ClassLoaders. | |||
* The value of the system property is simply best effort, as there is no way to faithfully represent | |||
* the Quarkus ClassLoader hierarchies in a system property value. | |||
* | |||
* @deprecated This was initially added to support Truffle, but it is no longer needed so the build item should not be used |
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.
Why is this part of this PR?
<plugins> | ||
<plugin> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-maven-plugin</artifactId> |
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.
In my case it was a problem to have CLI be a quarkus app. Check you can build from scratch.
import picocli.CommandLine.Parameters; | ||
|
||
@TopCommand | ||
@Command(name = "dockerfiles", sortOptions = false, mixinStandardHelpOptions = false, header = "Generate Dockerfiles.", headerHeading = "%n", commandListHeading = "%nCommands:%n", synopsisHeading = "%nUsage: ", optionListHeading = "%nOptions:%n") |
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.
@Command(name = "dockerfiles", sortOptions = false, mixinStandardHelpOptions = false, header = "Generate Dockerfiles.", headerHeading = "%n", commandListHeading = "%nCommands:%n", synopsisHeading = "%nUsage: ", optionListHeading = "%nOptions:%n") | |
@Command(name = "dockerfiles", sortOptions = false, mixinStandardHelpOptions = false, header = "Generate Dockerfiles/Containerfiles.", headerHeading = "%n", commandListHeading = "%nCommands:%n", synopsisHeading = "%nUsage: ", optionListHeading = "%nOptions:%n") |
@Option(names = { "--native" }, paramLabel = "", order = 5, description = "Flag to enable Native Dockerfile generation") | ||
boolean generateNativeDockerfile; | ||
|
||
@Parameters(arity = "0..1", paramLabel = "GENERATION_PATH", description = " The path to generate Dockerfiles") |
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.
@Parameters(arity = "0..1", paramLabel = "GENERATION_PATH", description = " The path to generate Dockerfiles") | |
@Parameters(arity = "0..1", paramLabel = "GENERATION_PATH", description = "The path to generate Dockerfiles") |
private static final ArtifactDependency QUARKUS_DOCKERFILES_SPI = new ArtifactDependency("io.quarkus", | ||
"quarkus-dockerfiles-spi", null, "jar", Dockerfiles.getVersion()); | ||
|
||
@Option(names = { "--jvm" }, paramLabel = "", order = 5, description = "Flag to enable JVM Dockerfile generation") |
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.
Does it generate all the variants or a single one (in this case, which one?)
@iocanel ping. |
I'll get back to it this week |
Updates:
quarkus-dockerfiles
as dependency