Skip to content

Conversation

@BarDweller
Copy link
Collaborator

Reported via : quarkusio/quarkus#47940

The cause was trying to use replaceAll to swap \ for /, and forgetting that replaceAll uses regex, so requires \\ not just \

However, it should have been possible to work around the issue by specifying host/socket as arguments, and that failed, because the sequence of invocations creating the config still drove the autodetect logic.

This moves the host/socket to a sub config object, ensuring they are set into the docker config object as a single operation.

This a breaking change to the config api, as withDockerHost & withDockerSocket are removed, and callers should use withHostAndSocketConfig instead

Also contains minor tweaks to ensure if both overrides are present, the autodetect logic is skipped entirely.

@BarDweller BarDweller self-assigned this May 21, 2025
@BarDweller BarDweller requested a review from cmoulliard May 21, 2025 17:51
@cmoulliard
Copy link
Member

This a breaking change to the config api, as withDockerHost & withDockerSocket are removed, and callers should use withHostAndSocketConfig instead

Should the examples be changed too ?

@BarDweller
Copy link
Collaborator Author

This a breaking change to the config api, as withDockerHost & withDockerSocket are removed, and callers should use withHostAndSocketConfig instead

Should the examples be changed too ?

The examples don't use withDockerHost or withDockerSocket, else they would be breaking the build =) They rely on auto-detect working..

We should probably add some new integration tests to verify behavior when overrides are used, but I'll add that as a separate PR.

@cmoulliard
Copy link
Member

LGTM

@BarDweller BarDweller merged commit 38fc715 into main May 23, 2025
101 of 102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants