Context switch - Plugin and Docker Desktop integration by simonferquel · Pull Request #1628 · docker/cli

simonferquel · 2019-01-21T08:55:33Z

- What I did

for plugins:
- Expose the Context Store config
- Make an existing config modifiable (to add endpoint types)
for Docker Desktop:
- Expose all context operations (except listing, which will be done by directly using context store functions)

- How I did it

Just exposed a few existing symbols.
Added a SetEndpoint function on store.Config

- How to verify it

Modified code covered by unit tests
Added a test for SetEndpoint, making sure it applied to already created context stores

ping @ijc for the plugin related part
ping @gtardif @pgayvallet @guillaumerose @ebriney for the Docker Desktop part

codecov-io · 2019-01-21T09:01:21Z

Codecov Report

Merging #1628 into master will decrease coverage by <.01%.
The diff coverage is 51.51%.

@@            Coverage Diff            @@
##           master   #1628      +/-   ##
=========================================
- Coverage    56.1%   56.1%   -0.01%     
=========================================
  Files         300     300              
  Lines       20578   20593      +15     
=========================================
+ Hits        11546   11554       +8     
- Misses       8202    8208       +6     
- Partials      830     831       +1

ijc

Some pretty minor docs nits, but otherwise looks good from the plugins PoV, thanks for taking care of this?

cli/command/cli.go

cli/command/context/create.go

cli/command/context/export.go

cli/command/context/remove.go

cli/command/context/update.go

simonferquel · 2019-01-21T11:06:22Z

Thanks for spotting the missing exported symbols, fixed

thaJeztah · 2019-01-22T14:33:50Z

(Copying this from #1519 (review));

The concern I have with exporting all of these is that;

These functions now become a public "API", with no interface defined for them
- this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")
Some of these functions actually have a "UX" component built-in (e.g. they print confirmation messages on stdout/stderr, have an option to print formatted output); is that really suitable for external consumption?
- calling these functions would become almost equivalent to exec.Cmd("docker stack ls")

For the underlying code on the other hand, we have various interfaces defined; if those are lacking functionality that's needed, should we review those interfaces, and make that the API to use?

simonferquel · 2019-01-22T15:39:15Z

Ok, I will try to move name validation and default context handling lower in the stack then

ijc · 2019-01-22T15:48:02Z

These functions now become a public "API", with no interface defined for them

this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")

I don't think that dichotomy really exists as strongly as you have stated it, it is entirely valid for software to export an API which is explicitly unstable if the maintainers choose to do so and for them to then feel no qualms about changing it as necessary despite it being exported. I think that would be an entirely valid approach to consider for something like docker/cli (at least the bits under discussion here and in #1519). It just means that external consumers should be aware and be prepared to adjust as the unstable interfaces change (e.g. while they are revendoring).

thaJeztah · 2019-01-23T14:05:46Z

Ah, sorry, I saw the comment on the other PR first, so let me copy my reply from there (#1519 (comment));

I was discussing with @chris-crone to add a note somewhere (perhaps the readme) to indicate that these functions, even though they're exported, should not be considered a stable API (as in; their signature can change, and shouldn't be relied on)

We could also add notes to the GoDoc of these, similar to https://golang.org/pkg/testing/#RunTests

An internal function but exported because it is cross-package; part of the implementation of the "go test" command.

But that would be a bit more work to do

I'm ok with this change, but we should add that note somewhere (separately from this PR), and start thinking of what changes would be needed to have a slightly higher-level interface than the API-client (to make it better consumable), but "lower" than the current functions (which, I think, are too tightly coupled to the CLI itself and the UX it has).

thaJeztah

LGTM

vdemeester

Just one "nit" 😅

vdemeester · 2019-01-23T16:39:15Z

cli/command/cli.go

+// ContextStoreConfig contains the config used by the context store
+// A plugin's init function may modify it to define other typed endpoint metadata by calling
+// ContextStoreConfig.SetEndpoint()
+var ContextStoreConfig = store.NewConfig(


Wouldn't it better to have a function here ?

(to be used from outside)

something like command.SetContextStoreEndpointType() ?

Or store.DefaultConfig() ? 🤔

simonferquel · 2019-01-24T12:50:20Z

As discussed privately with @vdemeester, I'll wait for Silvin's pr to land and introduce a WithAdditionalEndpoint option

thaJeztah · 2019-01-24T13:14:25Z

As discussed privately with @vdemeester, I'll wait for Silvin's pr to land and introduce a WithAdditionalEndpoint option

❤️ even better!

simonferquel · 2019-01-28T15:03:43Z

@thaJeztah done! PTAL :)

simonferquel · 2019-01-29T09:05:05Z

@vdemeester WDYT?

vdemeester

LGTM 🐯

thaJeztah

changes LGTM; can you squash commits?

This will allow plugins to have custom typed endpoints, as well as create/remove/update contexts with the exact same results as the main CLI (thinking of things like `docker ee login https://my-ucp-server --context ucp-prod)` Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

simonferquel · 2019-01-29T10:22:50Z

@thaJeztah rebased + squashed, thanks

thaJeztah

LGTM, thanks!

GordonTheTurtle added the status/0-triage label Jan 21, 2019

ijc reviewed Jan 21, 2019

View reviewed changes

cli/command/cli.go Outdated Show resolved Hide resolved

ijc reviewed Jan 21, 2019

View reviewed changes

cli/command/context/create.go Outdated Show resolved Hide resolved

guillaumerose reviewed Jan 21, 2019

View reviewed changes

cli/command/context/export.go Outdated Show resolved Hide resolved

guillaumerose reviewed Jan 21, 2019

View reviewed changes

cli/command/context/remove.go Outdated Show resolved Hide resolved

guillaumerose reviewed Jan 21, 2019

View reviewed changes

cli/command/context/update.go Outdated Show resolved Hide resolved

thaJeztah added status/2-code-review and removed status/0-triage labels Jan 23, 2019

thaJeztah approved these changes Jan 23, 2019

View reviewed changes

vdemeester requested changes Jan 23, 2019

View reviewed changes

simonferquel force-pushed the context-switch-plugin-and-desktop branch from 67f15ee to 64d01ca Compare January 28, 2019 15:00

vdemeester approved these changes Jan 29, 2019

View reviewed changes

thaJeztah requested changes Jan 29, 2019

View reviewed changes

simonferquel force-pushed the context-switch-plugin-and-desktop branch from 64d01ca to 3126920 Compare January 29, 2019 10:22

thaJeztah approved these changes Jan 29, 2019

View reviewed changes

thaJeztah merged commit a17f67e into docker:master Jan 29, 2019

GordonTheTurtle added this to the 19.03.0 milestone Jan 29, 2019

thaJeztah mentioned this pull request Aug 29, 2025

cli/command/context: deprecate exported types and functions #6402

Merged

thaJeztah mentioned this pull request Aug 29, 2025

[28.x backport] cli/command/context: deprecate exported types and functions #6403

Merged

Conversation

simonferquel commented Jan 21, 2019

Uh oh!

codecov-io commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ijc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonferquel commented Jan 21, 2019

Uh oh!

thaJeztah commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonferquel commented Jan 22, 2019

Uh oh!

ijc commented Jan 22, 2019

Uh oh!

thaJeztah commented Jan 23, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

vdemeester Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

simonferquel Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

simonferquel commented Jan 24, 2019

Uh oh!

thaJeztah commented Jan 24, 2019

Uh oh!

simonferquel commented Jan 28, 2019

Uh oh!

simonferquel commented Jan 29, 2019

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

simonferquel commented Jan 29, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Jan 21, 2019 •

edited

Loading

thaJeztah commented Jan 22, 2019 •

edited

Loading