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

feat: Get Docker endpoint from Docker context #1235

Merged
merged 26 commits into from
Sep 6, 2024

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Aug 19, 2024

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

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.

This enables seamless integration with alternative Docker engines such as [Colima][1] or [OrbStack][2].

Without this change it requires a [specific configuration][3] that's far from obvious to figure out.

[1]: https://github.com/abiosoft/colima
[2]: https://orbstack.dev
[3]: testcontainers/testcontainers-java#5034 (comment)
Copy link

netlify bot commented Aug 19, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 72938db
🔍 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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kiview
Copy link
Member

kiview commented Aug 19, 2024

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.

@0xced
Copy link
Contributor Author

0xced commented Aug 19, 2024

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.

@0xced 0xced changed the title Get the current docker endpoint through the docker context command Get the current docker endpoint by reading Docker configuration Aug 19, 2024
@0xced
Copy link
Contributor Author

0xced commented Aug 20, 2024

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. 🤷‍♀️

@HofmeisterAn
Copy link
Collaborator

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.

@HofmeisterAn
Copy link
Collaborator

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.

@0xced
Copy link
Contributor Author

0xced commented Aug 27, 2024

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.

@HofmeisterAn
Copy link
Collaborator

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.

@HofmeisterAn
Copy link
Collaborator

Ready for final review/merge?

@0xced
Copy link
Contributor Author

0xced commented Sep 6, 2024

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 (266ee03) for the test. Hope you don't mind bringing in the SkippableFact dependency.

So yes, ready for final review/merge.

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Sep 6, 2024
@HofmeisterAn HofmeisterAn changed the title Get the current docker endpoint by reading Docker configuration feat: Get Docker endpoint from Docker context Sep 6, 2024
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

💪

@HofmeisterAn HofmeisterAn merged commit 5a9f8f1 into testcontainers:develop Sep 6, 2024
11 checks passed
@0xced 0xced deleted the docker-context branch September 18, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants