testcontainers-dotnet
testcontainers-dotnet copied to clipboard
Get the current docker endpoint by reading Docker configuration
What does this PR do?
This pull request enables seamless integration with alternative Docker engines such as Colima or OrbStack.
These alternative engines make use of the Docker contexts feature to set themselves as the current Docker context.
Why is it important?
Without this change it requires a specific configuration that's far from obvious to figure out. It took me several hours fighting NullReferenceException because I'd only set the DOCKER_HOST environment variable without setting TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE. The ryuk container would fail to start causing the NullReferenceException.
Related issues
- Relates #930
How to test this PR
A new test has been added (DockerProcessTest.GetCurrentEndpoint) but installing Colima or OrbStack and actually using it with Testcontainers is the one true way to test this pull request. I did it with both Colima and OrbStack and it works on my machine.
Deploy Preview for testcontainers-dotnet ready!
| Name | Link |
|---|---|
| Latest commit | 72938db2215b662648006a2717543aafb4d1173d |
| Latest deploy log | https://app.netlify.com/sites/testcontainers-dotnet/deploys/66db4fcc82e35b0009978460 |
| Deploy Preview | https://deploy-preview-1235--testcontainers-dotnet.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Hi @0xced, this looks to me, like this is relying on a Docker CLI to inspect the context. We normally don't integrate with Docker (or other container runtimes) through the Docker CLI. There is the possibility to read the context directly from the file system (since I assume Docker.DotNet won't support context resolution), I remember @eddumelendez looking into this in the past.
I've address this concern in the follow-up commits by reading the configuration from the file system, like DockerRegistryAuthenticationProvider does.
I re-used the code that performs docker context inspect --format {{.Endpoints.docker.Host}} but in DockerConfigTest.GetCurrentEndpoint() to ensure that the implementation that reads the file system matches what the docker inspect command returns.
I think I finally got an implementation that matches the docker context inspect --format {{.Endpoints.docker.Host}} command invocation, that was quite some work. 😓
Fortunately the Docker CLI source code is well commented!
I don't like the many catch { return null; } parts but we wouldn't want to throw at that stage where we're trying to figure out the default endpoint to use. 🤷♀️
Hey, very cool pull request 😎, thanks! I've been thinking about supporting this feature for a while, so it's awesome to see it.
I added GetDockerContext() to the ICustomConfiguration interface to keep the implementation consistent when accessing configurations. I plan to add more documentation in the next few days, especially for https://dotnet.testcontainers.org/, since it's lacking attention at the moment.
After that, we can go ahead and merge the PR. Thank you once more.
I don't like the many
catch { return null; }parts but we wouldn't want to throw at that stage where we're trying to figure out the default endpoint to use. 🤷♀️
I removed them. Let's see how many issues pop up. I think I've covered most error paths (except invalid JSON). I'm not a huge fan of swallowing errors because it makes it hard to help and provide support on Slack, etc. when we don't know the actual configuration and the potential underlying issue. It would be nice to have a logger available, but the auto-discovery mechanism runs too early.
Please don't merge yet, I have found issues with the tests (the DockerConfig class is not well testable because of its static path properties). If you have a custom context some tests will fail locally but they will pass on CI as the current context will be the default one.
Please don't merge yet, I have found issues with the tests (the DockerConfig class is not well testable because of its static path properties). If you have a custom context some tests will fail locally but they will pass on CI as the current context will be the default one.
We can overload the ctor to also accept the default .docker and meta base directory paths. This would allow us to eliminate the GetDockerConfigFile() method too and read the path directly from customConfigurations.
Ready for final review/merge?
I just ran the test one more time on my machine and found a way where a test could fail. I just pushed a fix (266ee03d60125ba509e0a92093c9b90d0a54048e) for the test. Hope you don't mind bringing in the SkippableFact dependency.
So yes, ready for final review/merge.