Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Active-context handling has unresolved context-name and TLS/SSH compatibility issues, with missing focused macOS tests.
Review effort: Lite
Findings: None
What changed in this PR
Updates Docker endpoint discovery on macOS to support active contexts and user socket fallbacks.
Changes:
- Adds Docker context inspection and socket fallback logic.
- Applies configuration to Docker clients, ping checks, and dry-run execution.
| File | Summary |
|---|---|
pkg/connectors/container_client.go |
Adds Docker host resolution and client integration. Requires context-name resolution, support for TLS/SSH metadata, focused macOS tests, and gofmt cleanup. |
cmd/testDryRun.go |
Configures the Docker host before dry-run Testcontainers usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hey @Harsh4902, Just opened this PR for the macOS Docker context discovery, all CI checks are green. |
…able Signed-off-by: ish-g09 <ig.valiente09@gmail.com>
84b947b to
31eec2e
Compare
| // ConfigureDockerHost ensures DOCKER_HOST points to an active Docker daemon socket. | ||
| // On macOS, Docker Desktop may not install the privileged /var/run/docker.sock symlink. | ||
| // If DOCKER_HOST is not explicitly set, we look for the user socket or inspect the active context. | ||
| func ConfigureDockerHost() error { |
There was a problem hiding this comment.
Hey @ish-g09 This is a good workaround but if you see the codebase carefully, we don't set any DOCKER_HOST value for our docker client. We are using docker's official go-sdk to connect with docker. So ideally a solution should be from the docker sdk side and not a fallback on our side.
| return nil | ||
| } | ||
| func NewDockerClient() (*containerClient, error) { | ||
| if err := ConfigureDockerHost(); err != nil { |
There was a problem hiding this comment.
So, this check should be there only if docker's go-sdk dosen't provide the solution for this. Otherwise it a overhead on us to manage things whenever something is got changed on their end.
There was a problem hiding this comment.
Hey @Harsh4902, totally fair point! I actually dug into the Docker Go SDK and re-read the Mac permission docs you linked to check if the SDK could handle this natively.
Interestingly, Docker's docs call out this exact scenario:
"If you don't enable this option, the symlink... is not created and you may have to explicitly set the DOCKER_HOST environment variable to /Users//.docker/run/docker.sock in the clients it is using. The Docker CLI relies on the current context to retrieve the socket path..."
Basically, contexts are managed only by the docker CLI tool, while the Go client SDK (client.FromEnv) just looks for DOCKER_HOST and otherwise hardcodes /var/run/docker.sock. It doesn't know how to resolve contexts on its own.
That's why setting DOCKER_HOST through this small fallback (just like we already do for Podman in ConfigurePodmanHost) seemed like the simplest way to get it working on modern Macs without bringing in heavy CLI packages.
Let me know what you think! Happy to explore other ideas if there's a cleaner way you prefer or incase i've missed something.
There was a problem hiding this comment.
@ish-g09 I tried to dig deeper and found Docker has already deprecated it's original github.com/docker/docker go library which we are using currently. They are building a completely new docker go-sdk. And which provides access to low lever docker API and also have cli context so it can work smoothly. But, this sdk is still not in it's v1.0.0 which is basically first stable release. So the adoption of this library is crucial now. Let's think how long can we work with this temporary solution. Then we can adapt that. But for the long run, we anyways have to migrate to new docker go-sdk.
There was a problem hiding this comment.
I am also trying to connect with moby maintianers, as old docker go sdk is now transformed into project moby. If they can provide some library level solution then it's well and good. Otherwise we have to go with temporary solution.
There was a problem hiding this comment.
Makes total sense @Harsh4902! Happy to wait and see what they suggest. If it turns out they don't have a library-level option right now, we can always revisit this approach as a bridge until the new SDK stabilizes.
Keep me posted on what they say!
Description
ConfigureDockerHostinpkg/connectors/container_client.goto helpmicrocks-cliconnect to Docker on macOS when the default/var/run/docker.sockisn't available.docker context inspectso it honors whatever context the user is actively running (Docker Desktop, Colima, OrbStack, etc.).$HOME/.docker/run/docker.sockif context inspection isn't available.DOCKER_HOSTis already set or if/var/run/docker.sockexists (like on Linux).NewDockerClient(),PingDockerHost(), andcmd/testDryRun.goso bothmicrocks startandmicrocks test --dry-runcan reach Docker smoothly on modern macOS setups.Related issue(s)
Fixes #537
AI Disclosure
The code changes were written and verified by me. I fully understand them. Took help of AI to check for vulnerabilities.
Testing
go build ./...: passesgo test ./...: all packages pass