Skip to content

[WIP] cli-plugins: look for plugins in experimental folder when experimental is enabled - #1902

Closed
tiborvass wants to merge 2 commits into
docker:masterfrom
tiborvass:plugin_experimental2
Closed

[WIP] cli-plugins: look for plugins in experimental folder when experimental is enabled#1902
tiborvass wants to merge 2 commits into
docker:masterfrom
tiborvass:plugin_experimental2

Conversation

@tiborvass

@tiborvasstiborvass commented May 22, 2019•
edited
Loading

Copy link
Copy Markdown
Collaborator

This is an alternative to #1898

Closes#1897#1898

Related to docker/docker-ce-packaging#332

This patch expands the cli plugin search paths by adding -experimental to each,
only if the CLI has experimental mode enabled.

To test that experimental plugins do not work by default:

$ ln -s plugins-linux-amd64 build/plugins-linux-amd64-experimental
$ vim ~/.config/docker.json # add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs"
$ make plugins
$ make binary
$ docker helloworld

To show it enabled:

DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld

Signed-off-by: Tibor Vass tibor@docker.com

@tiborvasstiborvass mentioned this pull request May 23, 2019
Closed
@tiborvass
tiborvassforce-pushed the plugin_experimental2 branch from 849209a to ca44e4bCompareMay 23, 2019 01:02
@codecov-io

codecov-io commented May 23, 2019•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1902 into master will decrease coverage by 0.02%.
The diff coverage is 32.35%.

@@ Coverage Diff @@## master #1902 +/- ##
==========================================
- Coverage 56.75% 56.73% -0.03% 
==========================================
Files 309 309 Lines 21680 21701 +21 ==========================================
+ Hits 12305 12311 +6 - Misses 8476 8489 +13 - Partials 899 901 +2

@tiborvasstiborvass mentioned this pull request May 23, 2019
Merged
andrewhsu
andrewhsu approved these changes May 23, 2019
View reviewed changes

@andrewhsuandrewhsu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

ijc
ijc reviewed May 23, 2019
View reviewed changes
Comment threadcli-plugins/manager/manager.go Outdated
dirs := make([]string, len(pluginDirs), len(pluginDirs)*2)
copy(dirs, pluginDirs)
for _, dir := range dirs {
pluginDirs = append(pluginDirs, dir+"-experimental")

@ijcijcMay 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This puts all experimental paths after all non-experimental paths, rather than interleving them, is that as intended?

IOW this is:

~/.docker/cli-plugins
/usr/local/lib/docker/cli-plugins
/usr/lib/docker/cli-plugins
~/.docker/cli-plugins-experimental
/usr/local/lib/docker/cli-plugins-experimental
/usr/lib/docker/cli-plugins-experimental

and not

~/.docker/cli-plugins
~/.docker/cli-plugins-experimental
/usr/local/lib/docker/cli-plugins
/usr/local/lib/docker/cli-plugins-experimental
/usr/lib/docker/cli-plugins
/usr/lib/docker/cli-plugins-experimental

I think it's deliberate but just wanted to check that the implications had been considered i.e. As a user I cannot now install an experimental update to something shipped in /usr/lib/docker/cli-plugins in my home dir.

@thaJeztahthaJeztahMay 23, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was thinking about this; if someone installs both an "experimental" and "stable" version of a plugin; which one should take precedence?

@ijcijcMay 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it should be precisely the first enabled plugin found in the search path, which is why the above distinction on the ordering might matter.

With the current layout I think there is no way to install an experimental version "over" an non-experimental one, other than by pretending it is non-experimental

@tiborvasstiborvassMay 23, 2019

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

We could do either, I thought this was not only easier to implement, but also made more sense. But happy to change it.

ijc
ijc reviewed May 23, 2019
View reviewed changes
Comment threadcli-plugins/manager/candidate_test.go Outdated
continue
}
t.Run(tc.name, func(t *testing.T) {
fmt.Println("toto", tc.c.Experimental())

@ijcijcMay 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left over deubg?

ijc
ijc reviewed May 23, 2019
View reviewed changes
Comment threadcli-plugins/manager/candidate_test.go
ijc
ijc reviewed May 23, 2019
View reviewed changes

@ijcijc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks ok, I left some minor comments/nits.

I think the stuff about e2e testing from #1902 (comment) applies to this variant applies here too but you were going to do that in a follow up so that's fine.

@thaJeztahthaJeztah changed the title cli-plugins: look for plugins in experimental folder when experimental is enabled[WIP] cli-plugins: look for plugins in experimental folder when experimental is enabledMay 23, 2019
@thaJeztah

thaJeztah commented May 23, 2019

Copy link
Copy Markdown
Member

I renamed this "WIP" for now; as there might be things to discuss w.r.t. Docker Desktop integration and some other concerns; lets do so on #1897

Tibor Vass added 2 commits May 23, 2019 13:00
Tibor Vass
cli-plugins: look for plugins in experimental folder when experimenta…
200fada
…l is enabled
This patch expands the cli plugin search paths by adding `-experimental` to each,
only if the CLI has experimental mode enabled.
To test that experimental plugins do not work by default:
```
$ ln -s plugins-linux-amd64 build/plugins-linux-amd64-experimental
$ vim ~/.config/docker.json # add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs"
$ make plugins
$ make binary
$ docker helloworld
```
To show it enabled:
```
DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld
```
Signed-off-by: Tibor Vass <tibor@docker.com>
Tibor Vass
cli-plugins: add test names for easier debugging
e39aa2d
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass
tiborvassforce-pushed the plugin_experimental2 branch from ca44e4b to e39aa2dCompareMay 23, 2019 13:11
@tiborvass

tiborvass commented May 23, 2019

Copy link
Copy Markdown
CollaboratorAuthor

@ijc Updated with fixes.

@tiborvass

tiborvass commented May 23, 2019

Copy link
Copy Markdown
CollaboratorAuthor

I can't retrigger validate, but validate passed locally for me.

@thaJeztah

thaJeztah commented May 23, 2019

Copy link
Copy Markdown
Member

restarted CI (you can restart after connecting GitHub in the user-settings in CircleCI)

@thaJeztah

thaJeztah commented May 23, 2019

Copy link
Copy Markdown
Member

closing as #1898 was merged

@tiborvass

tiborvass commented May 23, 2019

Copy link
Copy Markdown
CollaboratorAuthor

I'm reopening because I thought this was still desired.

@tiborvasstiborvass reopened this May 23, 2019
@ijc

ijc commented May 24, 2019

Copy link
Copy Markdown
Contributor

I'm reopening because I thought this was still desired.

Yeah, I was about to say I don't think the two approaches need to be mutually exclusive...

@thaJeztah

thaJeztah commented Jul 20, 2021

Copy link
Copy Markdown
Member

Closing this one as #2774 enabled experimental features by default, and #2773 deprecated the associated options

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztahthaJeztahthaJeztah left review comments
@tonistiigitonistiigitonistiigi approved these changes
+3 more reviewers
@ijcijcijc left review comments
@andrewhsuandrewhsuandrewhsu approved these changes
@hc0503hc0503hc0503 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/0-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

docker cli experimental flag for plugins

8 participants

@tiborvass@codecov-io@thaJeztah@ijc@andrewhsu@tonistiigi@hc0503@GordonTheTurtle