-
-
Notifications
You must be signed in to change notification settings - Fork 515
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: prefer cross-platform default DOCKER_HOST #1294
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Given the Docker client already does what I suggested in https://github.com/testcontainers/testcontainers-go/pull/1294/files#r1231789163, I think it's safer to honour the Docker client, as proposed in this PR.
Shaun approves 😎
6e96fd4
to
9bb74ca
Compare
Instead of blindly assuming a Linux operating system we can rely on the docker client package to tell us the appropriate default docker host.
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM, thanks!
In the case we detected this fails once we are able to test the project on Windows workers, I'll let you know to collaborate on an eventual fix.
Thank you so much for your dedication to testcontaines-go! 💪 🚀
@danielorbach Hi, |
@mehdihadeli we are going to release a new version soon, hopefully at the end of this week |
Thanks |
@mdelapenya I haven't a clue how to do this 😅 |
Hey @danielorbach we core maintainers take care of the release process for the libraries, so no worries :) In any case, I've just performed the release: https://github.com/testcontainers/testcontainers-go/releases/tag/v0.21.0 |
Instead of blindly assuming a Linux operating system we can rely on the docker client package to tell us the appropriate default docker host.
What does this PR do?
I've changed the default
DOCKER_HOST
socket from its Linux value by relying on Docker's client library to supply their cross-platform values.I chose to parse their value as a URL (I say
parseURL
doing the same) to separate between the schema and the path.I was defensive because I am unfamiliar with the codebase - that's the reason for the suffix/prefix validations. Also, upon errors (url parsing, or unknown schema) I fallback to the predefined default rendering the changes of this PR a no-op.
Why is it important?
Windows users really want to use test-containers too! 🪟🐳
Before release
v0.20.1
the host was not explicitly set to a default, hence it fell-back to the docker library's default anyways.Related issues
How to test this PR
I haven't been able to run all the tests because I currently use a Windows machine. However, I ran some container-based tests (that broke with release
v0.20.1
) on my machine and they have passed 🤗