-
Notifications
You must be signed in to change notification settings - Fork 296
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
fix(core): Fix retrieval of the docker socket path when using rootless docker #710
base: main
Are you sure you want to change the base?
Conversation
…s docker (required to run ryuk). Fixes testcontainers#537
@@ -49,7 +50,6 @@ class TestcontainersConfiguration: | |||
ryuk_image: str = RYUK_IMAGE | |||
ryuk_privileged: bool = RYUK_PRIVILEGED | |||
ryuk_disabled: bool = RYUK_DISABLED | |||
ryuk_docker_socket: str = RYUK_DOCKER_SOCKET |
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 need to leave this configurable because otherwise people want to monkeypatch our modules
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.
Can't they just use the environment variable to configure it if someone ever needs it? A quick search on GitHub shows no-one is actually calling TestcontainersConfiguration
with custom params
Otherwise we can keep c.ryuk_docker_socket
as a direct property for people to configure
And rename the new computed property to something else, e.g. c.docker_socket
or c.get_docker_socket()
(and use this computed property when needing to provide the docker socket)
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.
see #531
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 don't really see where is the problem, it is possible to monkeypatch a computed property:
from testcontainers.core.config import testcontainers_config, TestcontainersConfiguration
def custom_ryuk_docker_socket(self) -> str:
return "/custom/path/to/docker.sock"
TestcontainersConfiguration.ryuk_docker_socket = property(custom_ryuk_docker_socket)
print(testcontainers_config.ryuk_docker_socket)
Otherwise I proposed another solution with a static property + a computed one, would this fit?
just add a TypedDict or something so its not just totally exposing another library API and I think that should be ok will take a look time permitting cc @kiview |
Currently the docker socket path used by
ryuk
is not working for rootless docker. We need to manually set it through theTESTCONTAINERS_DOCKER_SOCKET_OVERRIDE
environment variable.When running locally the
DOCKER_HOST
is always the full URL to the docker socket, e.g. on linux/macos, it's the socket path prefixed withunix://
andnpipe://
on windows. So we can easily extract the socket path using urllib parse:I improved the
TestcontainersConfiguration
so that the docker socket is inferred from the docker host when defined (and when theRYUK_DOCKER_SOCKET
env is not explicitly defined)The advantage of this approach is that it is automatically inferred from the
DOCKER_HOST
most of the time without the need for tedious manual configuration. And users can still explicitly define it for edge use-cases like before (e.g. when using docker over the network)I moved
get_docker_host()
to make it part of theTestcontainerConfig
, it was used at only 2 places, so it did not required many changes. And it makes sense to be part of the configAlternative solution
An alternatively solution has been proposed in #537. But it would rely on many uncertain and moving parts:
"/run/user/{user_id}/docker.sock"
which might change in the future or be different depending on the OS and configUID
env variable, not sure if this works on macos, and it would probably not work on windows.This solution could be implemented in
container.py
, e.g.:And would require to add
client.info()
indocker_client.py
Fixes #537
@alexanderankin