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.

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

Auto-detect container runtime #13669

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Conversation

gastaldi
Copy link
Contributor

@gastaldi gastaldi commented Dec 3, 2020

This checks that podman or docker is chosen based on the environment

This is a follow-up to #10637 (comment)

@ghost ghost added the area/core label Dec 3, 2020
@gastaldi gastaldi requested review from gsmet, geoand and zakkak December 3, 2020 15:12
@gastaldi gastaldi force-pushed the detect_podman branch 2 times, most recently from 4dd3903 to 5f32dcd Compare December 3, 2020 15:34
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

This checks that the podman or docker is chosen based on the environment
@gastaldi gastaldi merged commit b0830c0 into quarkusio:master Dec 3, 2020
@gastaldi gastaldi deleted the detect_podman branch December 3, 2020 21:45
@ghost ghost added this to the 1.11 - master milestone Dec 3, 2020
Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

@gastaldi just a minor issue in the javadoc of detectContainerRuntime

@@ -394,6 +397,51 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa
return nativeImage;
}

/**
* @return {@link ContainerRuntime#PODMAN} if the podman executable exists in the environment or if the docker executable
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment implies that podman is preferred over docker but the code does the opposite.

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, that was the initial behavior, but I changed later on and forgot to update the Javadoc 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

#13699 fixes this

@gsmet
Copy link
Member

gsmet commented Dec 4, 2020

While nice, this is a new feature, I won't backport it.

@gastaldi
Copy link
Contributor Author

gastaldi commented Dec 4, 2020

Actually I think it's more like a bug fix than a new feature, but that's ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants