-
-
Notifications
You must be signed in to change notification settings - Fork 492
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: support reading DOCKER_HOST from testcontainers props file #373
feat: support reading DOCKER_HOST from testcontainers props file #373
Conversation
docker.go
Outdated
return "" | ||
} | ||
tcProp := path.Join(home, ".testcontainers.properties") | ||
content, err := os.ReadFile(tcProp) |
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 should use a backwards compatible way of reading files OR properly define a support matrix
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 62.66% 61.90% -0.76%
==========================================
Files 15 15
Lines 1015 1042 +27
==========================================
+ Hits 636 645 +9
- Misses 280 294 +14
- Partials 99 103 +4
Continue to review full report at Codecov.
|
This includes support for reading docker specific properties, as specified in https://www.testcontainers.org/features/configuration/#customizing-docker-host-detection
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 from the perspective of aligning more closely with testcontainers-java
, thanks 👍
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.
👍
To fix the code coverage warnings in this PR, I think we should add a remote docker daemon, configure it with TLS, and create a DockerProvider struct with a TC properties pointing to that remote. Do you think we can live without that test? |
@mdelapenya given the complexity of such test that would delay this PR by a significant amount of time, I suggest we stick to the manual testing of it. That's what we also do in testcontainers-java :) |
@gianarb if you agree, I'd like to merge this one by the end of today. Otherwise feel free to add your thoughts Thanks! |
What does this PR do?
It adds support for retrieving the Docker host (remote or local) reading the DOCKER_HOST value from Testcontainers' default properties file (which is used at testcontainers-java for quite some time).
That file is located at
~/.testcontainers.properties)
.The PR includes unit tests for the new methods reading the props file.
Why is it important?
There are two main reasons:
Follow-up concerns
I've not tested this feature with Docker Compose yet (hopefully I can get some feedback from you all first)