Add plugin create functionality. by anusha-ragunathan · Pull Request #28164 · moby/moby

anusha-ragunathan · 2016-11-08T15:36:01Z

- What I did
Add plugin create functionality.

- How I did it
Given a rootfs and a plugin manifest.json, create a plugin.

- How to verify it
Use docker plugin create to create the plugin. Once successfully create, you can enable it and use it. You can also docker plugin push to registry.

Signed-off-by: Anusha Ragunathan anusha@docker.com

anusha-ragunathan · 2016-11-08T15:37:51Z

cc @vieux @tiborvass

AkihiroSuda · 2016-11-08T16:00:45Z

nit: ROOFS->ROOTFS

AkihiroSuda · 2016-11-08T16:00:54Z

AkihiroSuda · 2016-11-08T16:03:03Z

There is no description about rootfs in docs/extend/manifest.md. Can you add it?

uh, I found that in docs/extend/index.md

Should manifest be renamed? #28148

@vieux ^^

I'll do it in #28148 if it ever get's merged. it's good like this for now

AkihiroSuda · 2016-11-08T16:14:36Z

How about using an image instead of a raw rootfs directory?

e.g.

$ git clone https://github.com/vieux/docker-volume-sshfs
$ cd docker-volume-sshfs
$ docker build -t vieux/docker-volume-sshfs-image .
$ docker plugin create vieux/docker-volume-sshfs --manifest /somewhere/manifest.json vieux/docker-volume-sshfs-image

vieux · 2016-11-08T17:35:10Z

@AkihiroSuda we discussed this already, we think it's a good idea, it's coming soon (in a separate PR)

vieux · 2016-11-08T19:50:39Z

@anusha-ragunathan won't this be logged twice ? once here and once in the calling function ?

vieux · 2016-11-08T22:17:38Z

LGTM ping @tiborvass

tiborvass · 2016-11-08T23:00:55Z

why isn't this returning a simple error ?

tiborvass · 2016-11-08T23:01:32Z

return err should be enough

tiborvass · 2016-11-08T23:04:36Z

Apart from small optional nit, LGTM

vieux · 2016-11-09T01:02:22Z

ping @thaJeztah

tiborvass · 2016-11-09T01:04:59Z

LGTM

thaJeztah

Comments inline

thaJeztah · 2016-11-09T01:13:03Z

Since this is an example, the header should be part of the example;

POST /plugins/plugin:latest/create HTTP/1.1 Content-Type: application/tar {% raw %} {{ TAR STREAM }} {% endraw %}

Also can you check if application/tar or application/x-tar is the correct header? We use x-tar in other endpoints; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.25.md#extract-an-archive-of-files-or-folders-to-a-directory-in-a-container

According to http://superuser.com/a/960710, there's still no official content-type for tar

application/tar is indeed not registered at http://www.iana.org/assignments/media-types/media-types.xhtml#application. Unregistered mime types are considered "experimental" and have an x- prefix. So it makes sense to replace with application/x-tar.

Ofcourse curl recognizes both types. Eg. curl -H "Content-Type: application/tar" -X POST http://localhost:9999/v1.25/plugins/testplugin/create --data-binary "@/tmp/pluginDir/context.tar"

Yes, they probably work both. If it doesn't make a difference to us, then I prefer x-tar to be consistent

thaJeztah · 2016-11-09T01:14:06Z

This line can probably be removed if the example is updated

thaJeztah · 2016-11-09T01:15:31Z

this should be "plugin create (experimental)"

in the new docs rendering, this is actually used to create the H1 heading

thaJeztah · 2016-11-09T01:16:10Z

The page title should be removed for the new docs system (it's generated from the metadata)

Fixed. The last 3 comments apply across all plugin md files. @thaJeztah, is there a tool to run to check if the md files confirm to the new format? I ask because there might be a lot more across docker/docker.

thaJeztah · 2016-11-09T01:17:23Z

for the new docs, the [ and ] should be removed

thaJeztah · 2016-11-09T01:20:42Z

Should manifest be renamed? #28148

@vieux ^^

thaJeztah · 2016-11-09T01:26:19Z

should this be POST /plugins/create?name=<plugin name> instead? Feels odd to have an endpoint named after a not-yet existing resource.

/cc @bfirsh

@thaJeztah : that's what we have for other plugin endpoints such as enable, disable, push and set as well. I can either fix it just for create here, or make a follow-up PR to fix them all.

@anusha-ragunathan it's fine for enable disable, it's an action on an existing resource. (container start and stop are the same)

but I see @thaJeztah's point, it's different for create, the resource doesn't yet exist. (same for image or container create, it's not in the URL)

👍 Standard REST for creating an object is POST /objects. The reasoning, I think:

You are performing an action against the resource that is the set of plugins. The resource /plugins/name does not exist yet.

Servers often generate an ID or name for you. (Not the case in this instance, but may as well be consistent.)

Consistency. All REST APIs do this, so you kinda expect this to work.

Also worth noting that the endpoint is almost always POST /objects, not POST /objects/create, even though lots of our API does the latter. Depends whether you want to be consistent with the current world or a potential better future world! See also #25015.

@thaJeztah : I went ahead and updated endpoint to plugin/create/name=<>, after being convinced about the non-existent object endpoint.

It should probably be /plugins/create?name=foo, as in with name as a GET parameter. To follow the reasoning in (1) above, that's performing an action against the resource /plugins/create, but with name as an additional option.

It would typically be in the request body, but in this case that's impossible.

(I would also argue this should be just POST /plugins, but I'm happy to fix this in one big go in #25015. ;)

@bfirsh: +1 for tidying up all APIs URLs in one shot :)

bfirsh · 2016-11-09T20:12:07Z

Any chance you could also update api/swagger.yaml with the new endpoint? You can probably copy an existing endpoint and modify it. It looks pretty similar to /images/load, for example.

anusha-ragunathan · 2016-11-09T23:04:07Z

@bfirsh @thaJeztah : Updated with review comments on API. PTAL

thaJeztah · 2016-11-09T23:06:17Z

typo s/tagis/tag is/

Perhaps use the same as for image build;

- **name** - A name and optional tag to apply for the plugin in the `name:tag format`. If you omit the `tag` the default `:latest` value is assumed.

same review showing up thrice. fast fingers ? :)

bfirsh

Nice, thanks! Looks like it was the old format, though – I've commented the fixes. :)

bfirsh · 2016-11-09T23:06:02Z

/plugins/create:

bfirsh · 2016-11-09T23:06:30Z

in: "query"

thaJeztah

one nit/suggestion, but LGTM otherwise

thaJeztah · 2016-11-09T23:13:58Z

oh, hold on: this should be prefixed with /v1.25 now

thaJeztah · 2016-11-09T23:14:13Z

same here (/v1.25)

Signed-off-by: Anusha Ragunathan <anusha@docker.com>

anusha-ragunathan · 2016-11-09T23:24:59Z

@bfirsh @thaJeztah : updated

thaJeztah

thanks! LGTM!

bfirsh · 2016-11-10T00:00:50Z

swagger.yaml LGTM!

thaJeztah · 2016-11-10T00:27:43Z

Do not merge until #27967 is merged

moving back to docs review to prevent trigger-happy's

Add plugin create functionality.

GordonTheTurtle added the status/0-triage label Nov 8, 2016

anusha-ragunathan added this to the 1.13.0 milestone Nov 8, 2016

anusha-ragunathan added the area/plugins label Nov 8, 2016

AkihiroSuda reviewed Nov 8, 2016

View reviewed changes

Comment thread docs/reference/commandline/plugin_create.md Outdated

AkihiroSuda Nov 8, 2016

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.

ditto

AkihiroSuda reviewed Nov 8, 2016

View reviewed changes

icecrime added rebuild/experimental and removed rebuild/experimental labels Nov 8, 2016

vieux assigned tiborvass and vieux Nov 8, 2016

anusha-ragunathan force-pushed the plugin-build branch 3 times, most recently from 6ecf061 to d7591e6 Compare November 8, 2016 19:48

vieux reviewed Nov 8, 2016

View reviewed changes

anusha-ragunathan force-pushed the plugin-build branch from d7591e6 to 5c4ed5d Compare November 8, 2016 19:56

tiborvass reviewed Nov 8, 2016

View reviewed changes

Comment thread cli/command/plugin/create.go Outdated

tiborvass Nov 8, 2016

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.

why isn't this returning a simple error ?

tiborvass reviewed Nov 8, 2016

View reviewed changes

Comment thread cli/command/plugin/create.go Outdated

tiborvass Nov 8, 2016

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.

return err should be enough

anusha-ragunathan force-pushed the plugin-build branch from 5c4ed5d to 3df961a Compare November 9, 2016 00:11

vieux added status/3-docs-review and removed status/0-triage labels Nov 9, 2016

thaJeztah requested changes Nov 9, 2016

View reviewed changes

anusha-ragunathan force-pushed the plugin-build branch from 3df961a to 5466e29 Compare November 9, 2016 20:39

anusha-ragunathan force-pushed the plugin-build branch 2 times, most recently from 185bfd2 to 7945c69 Compare November 9, 2016 22:59

thaJeztah reviewed Nov 9, 2016

View reviewed changes

bfirsh suggested changes Nov 9, 2016

View reviewed changes

thaJeztah reviewed Nov 9, 2016

View reviewed changes

Comment thread docs/reference/api/docker_remote_api_v1.25.md Outdated

thaJeztah Nov 9, 2016

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.

same here (/v1.25)

anusha-ragunathan force-pushed the plugin-build branch 2 times, most recently from 2978dac to 0828a01 Compare November 9, 2016 23:18

Add plugin create functionality.

5b6e1bc

Signed-off-by: Anusha Ragunathan <anusha@docker.com>

anusha-ragunathan force-pushed the plugin-build branch from 0828a01 to 5b6e1bc Compare November 9, 2016 23:23

thaJeztah approved these changes Nov 9, 2016

View reviewed changes

thaJeztah added impact/api impact/changelog impact/cli impact/documentation labels Nov 9, 2016

bfirsh approved these changes Nov 10, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/3-docs-review labels Nov 10, 2016

thaJeztah added status/3-docs-review and removed status/4-merge labels Nov 10, 2016

vieux merged commit 28a1ea3 into moby:master Nov 10, 2016

anusha-ragunathan deleted the plugin-build branch November 10, 2016 17:00

srust mentioned this pull request Nov 21, 2016

[1.13.0-rc1] docker plugin create does not check for duplicates, creates multiple copies #28684

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28164 from anusha-ragunathan/plugin-build

1a34843

Add plugin create functionality.

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28164 from anusha-ragunathan/plugin-build

98408aa

Add plugin create functionality.

Uh oh!

Conversation

anusha-ragunathan commented Nov 8, 2016

Uh oh!

anusha-ragunathan commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 8, 2016

Uh oh!

vieux commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vieux commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Nov 8, 2016

Uh oh!

vieux commented Nov 9, 2016

Uh oh!

tiborvass commented Nov 9, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfirsh Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfirsh commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bfirsh Nov 9, 2016 •

edited

Loading

bfirsh commented Nov 9, 2016 •

edited

Loading

thaJeztah Nov 9, 2016 •

edited

Loading