cli/command: DockerCli.Initialize: make sure context-store config is set #5983
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In most situations, the CLI is created through the
NewDockerCli
constructor, however, it's possible to construct a CLI manually (&DockerCli{}
). We should probably prevent this (and un-export theDockerCli
implementation), but currently have some code-paths that depend on the type being exported.When constructing the CLI with this approach, the CLI would not be fully initialized and not have the context-store configuration set up.
Using the default context store without a config set will result in Endpoints
from contexts not being type-mapped correctly, and used as a generic
map[string]any
, instead of a docker.EndpointMeta.When looking up the API endpoint (using EndpointFromContext), no endpoint will be found, and a default, empty endpoint will be used instead which in its turn, causes newAPIClientFromEndpoint to be initialized with the default config instead of settings for the current context (which may mean; connecting with the wrong endpoint and/or TLS Config to be missing).
I'm not sure if this situation could happen in practice, but it caused some of our unit-tests (TestInitializeFromClient among others) to fail when running outside of the dev-container on a host that used Docker Desktop's "desktop-linux" context. In that situation, the test would produce the wrong "Ping" results (using defaults, instead of the results produced in the test).
This patch:
Initialize
function to set the default context-store config if no config was found (technically the field is mostly immutable, and can only set throughWithDefaultContextStoreConfig
, so this may be slightly redundant).We should update this code to be less error-prone to use; the combination of an exported type (
DockerCli
), a constructorNewDockerCli
and aInitialize
function (as well as some internal contructors to allow lazy initialization) make constructing the "CLI" hard to use, and there's various codepaths where it can be in a partially initialized state. The same applies to the default context store, which also requires too much "domain" knowledge to use properly.I'm leaving improvements around that for a follow-up.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)