docker / cli

The Docker CLI
Apache License 2.0
4.75k stars 1.88k forks source link

context list: temporarily add ContextType to JSON output #5094

Closed thaJeztah closed 1 month ago

thaJeztah commented 1 month ago

Docker Desktop currently ships with the "cloud integration" wrapper, which outputs an additional ContextType field in the JSON output.

While this field is non-standard, it made its way into Visual Studio's Docker integration, which uses this to exclude "aci" and "eci" context types that are not supported by Visual Studio.

This patch;

- Description for the changelog

Add a temporary `ContextType` field to the JSON output of "docker context ls" for backward-compatibility with Visual Studio.

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 61.37%. Comparing base (3da25f6) to head (fed9fa0).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5094 +/- ## ======================================= Coverage 61.37% 61.37% ======================================= Files 298 298 Lines 20707 20718 +11 ======================================= + Hits 12708 12716 +8 - Misses 7099 7102 +3 Partials 900 900 ```
thaJeztah commented 1 month ago

Interesting; for some reason static check does NOT complain about using deprecated field 🤔 wondering if we have some config incorrect in golangci-lint; I would expect it to barf;

8.45 cli/command/context/list.go:70:52: directive `//nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.` is unused for linter "staticcheck" (nolintlint)
68.45               ContextType: getContextType(nil, opts.format), //nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.
68.45                                                              ^
68.45 cli/command/context/list.go:87:69: directive `//nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.` is unused for linter "staticcheck" (nolintlint)
68.45           ContextType: getContextType(meta.AdditionalFields, opts.format), //nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.
68.45                                                                            ^
68.45 cli/command/context/list.go:105:51: directive `//nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.` is unused for linter "staticcheck" (nolintlint)
68.45           ContextType: getContextType(nil, opts.format), //nolint:staticcheck // ignore SA1019: field is deprecated, but used for backward-compatibility.
krissetto commented 1 month ago

The binary with this patch will only be used with DD for the upcoming release (and maybe one after that?), is that correct?

thaJeztah commented 1 month ago

Yes(ish); I wanted to avoid having to build some forked version of the CLI, so tried to create a patch with as minimal-as-possible impact, so that we could included it in a v26.1.4 patch release.

krissetto commented 1 month ago

so that we could included it in a v26.1.4 patch release.

if this does go out to "everyone" (and not only dd users), we need to document this in a visible way and be ready in case anything else breaks (i guess, just in case any other project depends on the shape of that json in even worse ways?)

thaJeztah commented 1 month ago

I added 3 commits to improve test-coverage; last commit is the change related to ContextType PTAL

thaJeztah commented 1 month ago

Maybe it's cleaner to move those 3 commits to a separate PR, so that this PR is only showing the changes related to ContextType; let me open a separate PR for the first 3 commits

thaJeztah commented 1 month ago

I'll rebase this one to get rid of the other commits

thaJeztah commented 1 month ago

Rebased and moved out of draft