Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move plugins out of experimental #28226

Merged
merged 2 commits into from Nov 11, 2016
Merged

move plugins out of experimental #28226

merged 2 commits into from Nov 11, 2016

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Nov 10, 2016

@vieux vieux added priority/P1 Important: P1 issues are a top priority and a must-have for the next release. status/2-code-review labels Nov 10, 2016
@vieux vieux added this to the 1.13.0 milestone Nov 10, 2016
@vieux vieux self-assigned this Nov 10, 2016
@thaJeztah
Copy link
Member

Can you check integration tests as well? Tests are probably skipped if experimental is not enabled

@vieux
Copy link
Contributor Author

vieux commented Nov 10, 2016

@thaJeztah right, thanks

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

/cc @albers @sdurrheimer because this will likely affect the completion scripts

@stevvooe
Copy link
Contributor

This cannot be merged until #28148 is merged.

@anusha-ragunathan
Copy link
Contributor

Please add 26760, which tracks this.

@@ -6,7 +6,7 @@ keywords: "API, Docker, rcli, REST, documentation"

<!-- This file is maintained within the docker/docker Github
repository at https://github.com/docker/docker/. Make all
pull requests against that repo. If you see this file in
5B pull requests against that repo. If you see this file in
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@anusha-ragunathan
Copy link
Contributor

tests that need to be updated:

  • docker_cli_daemon_experimental_test.go (all of them)
  • docker_cli_events_test.go (TestEventsPluginOps)
  • docker_cli_authz_plugin_v2_test.go (all of them)
  • docker_cli_network_unix_test.go (TestDockerPluginV2NetworkDriver)

@anusha-ragunathan
Copy link
Contributor

daemon/daemon_experimental.go needs to remove the daemon.HasExperimental checks and merge with daemon/daemon.go. Else nothing will work.

* `GET /plugins/(plugin name)` inspect a plugin.
* `POST /plugins/(plugin name)/set` configure a plugin.
* `POST /plugins/(plugin name)/enable` enable a plugin.
* `POST /plugins/(plugin name)/disable` disable a plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add plugin create:
``POST /plugins/create?name=plugin name creates a plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also missing push

@@ -184,6 +184,14 @@ This section lists each version from latest to oldest. Each listing includes a
* `POST /services/create` and `POST /services/(id or name)/update` now accept the `TTY` parameter, which allocate a pseudo-TTY in container.
* `POST /services/create` and `POST /services/(id or name)/update` now accept the `DNSConfig` parameter, which specifies DNS related configurations in resolver configuration file (resolv.conf) through `Nameservers`, `Search`, and `Options`.
* `GET /networks/(id or name)` now includes IP and name of all peers nodes for swarm mode overlay networks.
* `GET /plugins` list plugins.
* `POST /plugins/pull?name=<plugin name>` install a plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be pulls a plugin. install implies an additional enable.

@@ -171,7 +170,6 @@ Config provides the base accessible fields for working with V0 plugin format

```
{
"configVersion": "v0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a doc left-over from a previous PR, it's already removed from the code, after talking with @tiborvass and @stevvooe.

  • You needed to json-decode in order to get the version, if the format change a lot, it won't even work.
  • The version is already present in the media-type, that's how other media handle versioning.

@vieux
Copy link
Contributor Author

vieux commented Nov 10, 2016

Thanks @anusha-ragunathan, I missed a lot indeed

@anusha-ragunathan
Copy link
Contributor

Thank you for doing this @vieux . I know you've been burning the midnight oil with a lotta plugin PRs!

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

* `POST /plugins/(plugin name)/set` configure a plugin.
* `POST /plugins/(plugin name)/enable` enable a plugin.
* `POST /plugins/(plugin name)/disable` disable a plugin.
* `POST /plugins/(plugin name)/push` push a pluging.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo pluging

@@ -153,7 +153,8 @@ func Pull(ref reference.Named, rs registry.Service, metaheader http.Header, auth
logrus.Debugf("pull.go: error in json.Unmarshal(): %v", err)
return nil, err
}
if m.Config.MediaType != schema2.MediaTypePluginConfig {
if m.Config.MediaType != schema2.MediaTypePluginConfig &&
m.Config.MediaType != "application/vnd.docker.plugin.image.v0+json" { //TODO: remove this v0 before 1.13 GA
Copy link
Contributor

Choose a reason for hiding this comment

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

A similar check for image.v0 needs to be added in https://github.com/vieux/docker/blob/exit_exp_plugin/distribution/pull_v2.go#L361, to avoid using docker pull to pull plugin images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@vieux vieux force-pushed the exit_exp_plugin branch 4 times, most recently from 0bc70b9 to 683167c Compare November 10, 2016 21:56
@anusha-ragunathan
Copy link
Contributor

LGTM (once tests are all green)

@tiborvass
Copy link
Contributor

@vieux needs rebase :(

@tiborvass
Copy link
Contributor

apart from small conflict, LGTM

@lowenna
Copy link
Member

lowenna commented Nov 10, 2016

As plugins don't work on Windows (at least I've never verified and don't believe they do anyway), can we get this updated to report a nice "not supported on this platform" message for the API endpoints? And document accordingly.

Signed-off-by: Victor Vieux <vieux@docker.com>
Signed-off-by: Victor Vieux <vieux@docker.com>
@vieux
Copy link
Contributor Author

vieux commented Nov 10, 2016

@jhowardmsft yes it's on my list of things to do during the freeze

@lowenna
Copy link
Member

lowenna commented Nov 11, 2016

Thanks, @vieux

@vieux vieux merged commit acd6e0d into moby:master Nov 11, 2016
@vieux vieux deleted the exit_exp_plugin branch November 11, 2016 01:30
@tomdee
Copy link
Contributor

tomdee commented Nov 16, 2016

@vieux Am I right in thinking that network plugins still aren't supported?

@mavenugo
Copy link
Contributor

mavenugo commented Nov 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/changelog impact/cli impact/documentation priority/P1 Important: P1 issues are a top priority and a must-have for the next release. status/4-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Pluginv2 out of experimental
10 participants