internal/core: add docker context inspect to host lookup strategies
#3171
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This fixes the user-facing panic, covers additional tests that should be reported as non-fatal, and most importantly includes `docker context inxpect` in the docker host resolution strategies. Fixes testcontainers#2952
tmc
force-pushed
the
use-docker-context-inspect
branch
from
923cb7b to
339bfde
Compare
There was a problem hiding this comment.
@tmc thanks for your contribution, I left a comment about the docker context resolution.
Other than that, I'm fine with not using the function that panics and let the client code handle the error, so if your agree, removing the docker context part from this PR would make it easier to merge asap.
Wdyt?
| // dockerHostFromDockerContext returns the docker host from the DOCKER_CONTEXT environment variable, if it's not empty | ||
| func dockerHostFromDockerContext(_ context.Context) (string, error) { | ||
| // exec `docker context inspect -f='{{.Endpoints.docker.Host}}'` | ||
| cmd := exec.Command("docker", "context", "inspect", "-f", "{{.Endpoints.docker.Host}}") |
There was a problem hiding this comment.
suggestion: we have an ongoing PR addressing the docker context support. Although shelling out to the docker CLI is easy, we disregard its usage to avoid handling exit codes, and parsing responses.
There was a problem hiding this comment.
That is an wild PR my guy! 😂.
The user-facing panic is unfortunate but secondary to not respecting the current docker context. If you have another fix for this incoming I can just close this PR.
I don't completely understand that rationale -- I'd buy not wanting to rely on on a docker binary on PATH but this package has lots of passing on failures if things don't work -- but that's fine you can do what you think is best.
I find layers of complexity on top of test code need to be abundantly clear clear that they are worth the complexity.
Obtaining the current docker host from the current context being more than a 30 line change is a big red flag IMO.
Regardless, I don't really mind how this gets fixed but it impacts me and folks contributing things I'm working on.
So far my experience with this package has driven me to write first a wrapper and then a (IMO) both much smaller, simpler, and better implementation that is much more natively tied into the go testing library. I haven't made it public but it will be at http://github.com/tmc/testctr at some point soon -- it has a (completely optional) testcontainers-go backend implementation.
Adds
docker context inspectto the host lookup strategies and fixes a user-facing panic in host resolution. The change properly handles errors instead of panicking and expands the set of non-fatal errors.Improves Docker host resolution reliability when using Docker contexts and provides better error handling for users.
This further prevents putting the possible panic on user-facing calling pathways.
Fixes #2952