-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add a rootless Docker strategy #2985
Conversation
core/src/main/java/org/testcontainers/dockerclient/RootlessDockerClientProviderStrategy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/ResourceReaper.java
Outdated
Show resolved
Hide resolved
@@ -21,6 +24,9 @@ | |||
|
|||
@Test | |||
public void shouldReportOOMAfterWait() { | |||
Info info = DockerClientFactory.instance().client().infoCmd().exec(); | |||
// Poor man's rootless Docker detection :D |
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.
😂 for our tests, could we do this using the host socket URI?
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 didn't want to do string matching, especially given that the feature is experimental and may change the socket location.
Apparently, Docker returns rootless
security capability from the info endpoint, just docker-java does not expose it (yet). I will add it to docker-java, so that we can remove this workaround, okay? :)
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.
Cool, I was wondering if that would be the case. Let's not hold up this PR for the change in docker-java, though - it's OK as-is.
@Deprecated | ||
public final class RootlessDockerClientProviderStrategy extends DockerClientProviderStrategy { | ||
|
||
public static final int PRIORITY = EnvironmentAndSystemPropertyClientProviderStrategy.PRIORITY + 20; |
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.
Do we want this to be the highest priority? I'd put this lower than UnixSocketClientProviderStrategy
, at least after #1771 is merged (so that DOCKER_HOST
is always respected, non-rootless-docker is tried first, and automatically configured rootless docker is tried last of all)
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 didn't want rootless DOCKER_HOST to take precedence (because we need to override the host ip) but yeah, I see what you mean.
I will make it have lower priority, but ensure that EnvAndSysPropsStrategy does not detect rootless host that easily
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.
update:
with overridable host, it is no longer a problem when EnvAndSysPropsStrategy
detects the rootless DOCKER_HOST
docs/features/configuration.md
Outdated
|
||
However, sometimes a customization is required. For that, you can provide the following environment variables: | ||
|
||
> **TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE = /var/run/docker-alt.sock** |
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 we make these match the format of other configuration settings, where we show what the default value is?
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.
We could, but there is no default value, so I am not sure what to use :D
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 great, thanks @bsideup!
Closes #2943, #1770