-
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
Extract constants for runtime and builder images #42652
Conversation
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview c75c0f4 has been successfully built and deployed to https://quarkus-pr-main-42652-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
d925da1
to
c8379e4
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.
Looks mostly good. I added some comments/concerns though.
Also wondering if we should adjust:
extensions/container-image/container-image-docker-common/deployment/src/test/java/io/quarkus/container/image/docker/common/deployment/RedHatOpenJDKRuntimeBaseProviderTest.java
extensions/container-image/container-image-docker-common/deployment/src/test/java/io/quarkus/container/image/docker/common/deployment/UbiMinimalBaseProviderTest.java
(I had a look at https://github.com/quarkusio/quarkus/pull/42097/files)
...hift/deployment/src/main/java/io/quarkus/container/image/openshift/deployment/S2iConfig.java
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/images/ContainerImages.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/images/ContainerImages.java
Show resolved
Hide resolved
@gsmet Any opinion about the reference located in the javadoc? |
This comment has been minimized.
This comment has been minimized.
@cescoffier could you clarify your question? If it's about the convention you put in the Javadoc, looks good. |
This avoids having to modify multiple places when there is an update. I decided to put the constants in core-deployment to avoid the runtime overhead (it's a lot of strings). We may revisit later.
c8379e4
to
6beef7a
Compare
I fixed the 2 locations you mentioned. |
Status for workflow
|
Status for workflow
|
This avoids having to modify multiple places when there is an update.
I decided to put the constants in core-deployment to avoid the runtime overhead (it's a lot of strings). We may revisit later.
Fix #42389