Skip to content

Conversation

@ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Oct 21, 2025

What does this PR do?

drop the context and config modules to rely on docker/cli to access the CLI config.

This prevents code duplication between those repositories and enforce docker/cli is in charge for the CLI config storage and can evolve without risks.
docker/cli is used as a dependency: it manages initialization of an APIClient and acts as a provider for registry credentials in a consistent way with other Docker tools (CLI, Compose, Buildx ...).

Why is it important?

Custom code to manage CLI config put us at risks for inconsistent behaviors, misalignment and extra maintenance efforts.

@ndeloof ndeloof changed the title Rely on docker/cli to get APIClient and registry credentials chore(config): Rely on docker/cli to get APIClient and registry credentials Oct 21, 2025
@ndeloof ndeloof force-pushed the dockerCLI branch 2 times, most recently from bdcdc70 to c3d932f Compare October 21, 2025 09:20
@ndeloof ndeloof changed the title chore(config): Rely on docker/cli to get APIClient and registry credentials chore: Rely on docker/cli to get APIClient and registry credentials Oct 21, 2025
@ndeloof ndeloof marked this pull request as ready for review October 22, 2025 11:18
Copilot AI review requested due to automatic review settings October 22, 2025 11:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes custom Docker CLI config handling code in favor of using the official docker/cli library as a dependency. This consolidation eliminates code duplication and ensures consistent behavior with other Docker tools like CLI, Compose, and Buildx by relying on docker/cli to manage APIClient initialization and registry credential access.

Key Changes:

  • Removed context, config, and legacyadapters modules entirely
  • Updated client initialization to use docker/cli's command package
  • Modified registry credential resolution to delegate to docker/cli

Reviewed Changes

Copilot reviewed 69 out of 77 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
client/client.go Replaced custom client initialization with docker/cli's command.NewDockerCli
client/registry.go Added new methods for auth config retrieval using docker/cli
client/types.go Added AuthConfigForImage and AuthConfigForHostname methods to SDKClient interface
image/options.go Updated credential retrieval to use new client methods instead of config package
container/image_substitutors.go Changed import from config/auth to image package
Multiple go.mod files Updated dependencies to include docker/cli and removed replaced modules
context/, config/, legacyadapters/* Removed entire modules and their implementations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

func (c *sdkClient) AuthConfigForHostname(host string) (registry.AuthConfig, error) {
config, err := c.config.GetAuthConfig(host)
if err != nil {
return registry.AuthConfig{}, nil
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

Error from GetAuthConfig is being silently ignored. When an error occurs, the function returns an empty AuthConfig without propagating the error, which could hide authentication failures or configuration problems. The error should be returned to the caller.

Suggested change
return registry.AuthConfig{}, nil
return registry.AuthConfig{}, err

Copilot uses AI. Check for mistakes.
Comment on lines 7 to 12
"github.com/docker/go-sdk/config/auth"
)

// AuthConfigForImage returns the auth config for a single image
func (c *sdkClient) AuthConfigForImage(image string) (string, registry.AuthConfig, error) {
ref, err := auth.ParseImageRef(image)
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

This import references the config package which is being removed in this PR. The import should be changed to use the image package instead, consistent with the changes in container/image_substitutors.go.

Suggested change
"github.com/docker/go-sdk/config/auth"
)
// AuthConfigForImage returns the auth config for a single image
func (c *sdkClient) AuthConfigForImage(image string) (string, registry.AuthConfig, error) {
ref, err := auth.ParseImageRef(image)
"github.com/docker/go-sdk/image"
)
// AuthConfigForImage returns the auth config for a single image
func (c *sdkClient) AuthConfigForImage(image string) (string, registry.AuthConfig, error) {
ref, err := image.ParseImageRef(image)

Copilot uses AI. Check for mistakes.
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM!

Massive changes, removing lot of code to be maintained. I agree this indirection layer can be removed if the CLI now is the source of truth. My initial idea was to be the other way around, but if we can make progress in the CLI more easily, then it's perfectly fine to delegate the responsibility of context & auth to the CLI.

I only have one question: IIUC the SDK client now receives a CLI client, right? https://github.com/docker/go-sdk/pull/109/files#diff-bd3a55a72186f59e2e63efb4951573b2f9e4a7cc98086e922b0859f8ccc1dd09R69

We must document it in the existing docs, as I think we explicitly mention that we are using the current docker context.

Another concern that I have: Is the current context and docker host resolution something accessible from the CLI side? I can see places where users could read the docker socket to mount it in a container:

dockerHost, err := gosdk_context.CurrentDockerHost()

Is that possible with this new approach?

Other than that, I think this PR is great, merci!

@mdelapenya mdelapenya changed the title chore: Rely on docker/cli to get APIClient and registry credentials chore: rely on docker/cli to get APIClient and registry credentials Oct 22, 2025
@mdelapenya mdelapenya changed the title chore: rely on docker/cli to get APIClient and registry credentials chore(client): rely on docker/cli to get APIClient and registry credentials Oct 22, 2025
@ndeloof
Copy link
Contributor Author

ndeloof commented Oct 22, 2025

we explicitly mention that we are using the current docker context

cli.Initialize(flags.NewClientOptions()) does the same, relying on resolveContextName to manage DOCKER_* env variables and local config

I can see places where users could read the docker socket to mount it in a container

Unfortunately this approach is wrong. socket as seen from client is not relevant to be mounted on docker host. This only works as long as both use default location. Typically, on Docker Desktop, you would mount /Users/xxx/.docker/run/docker.sock inside container, which relies on a cross-VM mount from MacOS host inside container to actually access a socket which lives inside VM 🤪

See https://github.com/docker/cli/blob/master/cli/command/container/create.go#L244-L259 and moby/moby#43459

IMHO it would make sense to mimic this docker/cli code and offer a WithAPISocket helper method to manage this mess (also need to inject user's registry credentials inside container to be consistent)

@mdelapenya
Copy link
Member

Unfortunately this approach is wrong. socket as seen from client is not relevant to be mounted on docker host. This only works as long as both use default location. Typically, on Docker Desktop, you would mount /Users/xxx/.docker/run/docker.sock inside container, which relies on a cross-VM mount from MacOS host inside container to actually access a socket which lives inside VM 🤪

See https://github.com/docker/cli/blob/master/cli/command/container/create.go#L244-L259 and moby/moby#43459

I like the opinionated way of discovering the docker socket using the Docker context: "give me a current context and I'll find the socket". It works OOTB for different container runtimes, which to me is the right thing to do: Orbstack, Colima, Podman, Rancher... I have tested it here: https://github.com/mdelapenya/go-sdk-examples

IMHO it would make sense to mimic this docker/cli code and offer a WithAPISocket helper method to manage this mess (also need to inject user's registry credentials inside container to be consistent)

Where should this code live, in the SDK or the CLI?

@ndeloof
Copy link
Contributor Author

ndeloof commented Oct 22, 2025

I like the opinionated way of discovering the docker socket using the Docker context

Docker Context gives you connexion details to docker host from client machine, which are not relevant to bind mount engine socket inside a container. Typically on a windows client, context will give you npipe:///pipe/docker_engine which will just fail. Same applies to a remote connexion using ssh. It works by chance as 99% users rely on the default /var/run/docker.sock path with a local (or Docker Desktop) engine. See docker/cli#6157

Where should this code live, in the SDK or the CLI?

I guess it could live on the SDK as this code is a temporary workaround anyway, waiting for engine API to offer an actual "useAPIsocket" option

* main:
  feat(wait): add human-readable String() methods to all wait strategies (docker#119)
  feat(image): display formatted pull progress on Windows terminals (docker#118)
  chore(release): add Go proxy refresh automation (docker#117)
  chore(release): bump module versions
  chore(release): prevent for pushing to personal forks (docker#115)
  chore(release): add pre-release checks to make the release process more consistent (docker#114)
  chore(volume): return pointer in FindByID (docker#105)
  fix(container): proper error message on empty container names (docker#113)
  fix(container): add to nil modifiers (docker#112)
  feat(container): add new functional options to add container config, hostConfig and endpointSettings (docker#111)
  feat(container): configure pull handler at container creation (docker#110)
* main:
  deps(go): bump Go to 1.24 (docker#120)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants