Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Trim container-image configuration values #19958

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Trim container-image configuration values #19958

merged 1 commit into from
Sep 7, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 7, 2021 •
edited
Loading

These values must be trimmed or else the container-image build fails.

Fixes: #19956

@geoand geoand requested a review from gsmet September 7, 2021 09:44
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 7, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 7, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 185c130

Status Name Step Failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Failures Logs Raw logs
✖ JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✖ JVM Tests - JDK 16 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/container-image/deployment 
! Skipped: docs extensions/container-image/container-image-docker/deployment extensions/container-image/container-image-jib/deployment and 12 more

📦 extensions/container-image/deployment

✖ io.quarkus.container.image.deployment.ContainerImageInfoTest.shouldUseAppNameAndVersionWhenNoUserName line 63 - More details - Source on GitHub

java.lang.IllegalArgumentException: The supplied combination of container-image group '' and name 'repo/name' is invalid
	at io.quarkus.container.image.deployment.ContainerImageProcessor.publishImageInfo(ContainerImageProcessor.java:77)
	at io.quarkus.container.image.deployment.ContainerImageInfoTest.whenPublishImageInfo(ContainerImageInfoTest.java:125)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/container-image/deployment 
! Skipped: docs extensions/container-image/container-image-docker/deployment extensions/container-image/container-image-jib/deployment and 12 more

📦 extensions/container-image/deployment

✖ io.quarkus.container.image.deployment.ContainerImageInfoTest.shouldUseAppNameAndVersionWhenNoUserName line 63 - More details - Source on GitHub

java.lang.IllegalArgumentException: The supplied combination of container-image group '' and name 'repo/name' is invalid
	at io.quarkus.container.image.deployment.ContainerImageProcessor.publishImageInfo(ContainerImageProcessor.java:77)
	at io.quarkus.container.image.deployment.ContainerImageInfoTest.whenPublishImageInfo(ContainerImageInfoTest.java:125)

⚙️ JVM Tests - JDK 16 #

- Failing: extensions/container-image/deployment 
! Skipped: docs extensions/container-image/container-image-docker/deployment extensions/container-image/container-image-jib/deployment and 12 more

📦 extensions/container-image/deployment

✖ io.quarkus.container.image.deployment.ContainerImageInfoTest.shouldUseAppNameAndVersionWhenNoUserName line 63 - More details - Source on GitHub

java.lang.IllegalArgumentException: The supplied combination of container-image group '' and name 'repo/name' is invalid
	at io.quarkus.container.image.deployment.ContainerImageProcessor.publishImageInfo(ContainerImageProcessor.java:77)
	at io.quarkus.container.image.deployment.ContainerImageInfoTest.whenPublishImageInfo(ContainerImageInfoTest.java:125)

@geoand
Copy link
Contributor Author

geoand commented Sep 7, 2021

Failures are related.
I'll take a look later

iocanel
iocanel approved these changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gsmet
gsmet reviewed Sep 7, 2021
View reviewed changes
...age/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java
@@ -102,6 +106,9 @@
if (group.isPresent()) {
String originalGroup = group.get();
if (originalGroup.equals(System.getProperty("user.name"))) {
if (originalGroup.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. So the converter makes the string always present?

Copy link
Contributor Author

@geoand geoand Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be the case... cc @radcortez

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is, that's definitely not what I expected. I would have expected the Optional to stay empty if no string is defined. Let's wait for @radcortez 's opinion on this :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that the Converter must return null if there is no value. From the Config rules, an empty String "" is automatically converted to null, but in the case of this TrimmedStringConverter the conversion is the empty String again, which is then passed to the Optional, so you get a non empty Optional.

The TrimmedStringConverter must return a null if the resulting String is an empty String. Now we may need to recheck other usages of this converter to make sure we don't break it.

@geoand geoand merged commit 7ccc007 into quarkusio:main Sep 7, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 7, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 7, 2021
@geoand geoand deleted the #19956 branch September 7, 2021 13:50
@joggeli34 joggeli34 mentioned this pull request Sep 24, 2021
Closed
@gsmet gsmet modified the milestones: 2.3.0.CR1, 2.2.4.Final Nov 30, 2021
@pjgg pjgg mentioned this pull request Jan 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radcortez radcortez radcortez left review comments

@gsmet gsmet gsmet approved these changes

@iocanel iocanel iocanel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.4.Final
Development

Successfully merging this pull request may close these issues.

Automatically remove trailing spaces in the application.properties file
4 participants
@geoand @iocanel @gsmet @radcortez