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

Adding Docker API endpoint to inspect image manifest #32061

Merged
merged 4 commits into from May 10, 2017

Conversation

@nishanttotla
Contributor

nishanttotla commented Mar 24, 2017

The goal of this PR is to make it possible for the client to figure out the image manifest, which will allow the client to construct the spec with the full digest (instead of doing it in the daemon like #28173) and platform information (#31348).

The client can make a call to the Engine to retrieve the manifest.

Here are more details about the change:

- What I did
Added /distribution/{name:*}/json endpoint in the API, that accesses the registry and pulls the manifest. This is required to pull digest and platform information.

- How I did it
I added a new distributionRouter, and a function called getDistributionInfo in distribution_routes.go that accesses the registry to get the digest, and pulls the image manifest to get more information. A DistributionInspect struct is returned.

- How to verify it
Send a curl request to the engine, at http:/distribution/alpine:latest/json

- Description for the changelog

Adding Docker API endpoint to inspect image manifest

- A picture of a cute animal (not mandatory but encouraged)
(\__/)
(='.'=)
(")_(")

I'm on a weak wifi, so here's a text bunny.

What I will do after this PR:

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
Contributor

nishanttotla commented Mar 25, 2017

@nishanttotla nishanttotla changed the title from [WIP] Adding endpoint to contact registry from engine-api to Adding endpoint to contact registry from engine-api Mar 29, 2017

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Mar 29, 2017

Contributor

Bumping up this PR. It works in the current form, and the goal is for the client to be able to use this endpoint to get manifest information to construct an appropriate spec.

Contributor

nishanttotla commented Mar 29, 2017

Bumping up this PR. It works in the current form, and the goal is for the client to be able to use this endpoint to get manifest information to construct an appropriate spec.

Show outdated Hide outdated api/server/router/image/image.go Outdated
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Mar 30, 2017

Contributor

Architecture information is updated for all types of manifests now.

cc @aaronlehmann

Contributor

nishanttotla commented Mar 30, 2017

Architecture information is updated for all types of manifests now.

cc @aaronlehmann

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 1, 2017

Contributor

@aaronlehmann addressed all comments.

Contributor

nishanttotla commented Apr 1, 2017

@aaronlehmann addressed all comments.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 1, 2017

Contributor

Design LGTM

Don't forget to update swagger.yml to document the new API endpoint.

Contributor

aaronlehmann commented Apr 1, 2017

Design LGTM

Don't forget to update swagger.yml to document the new API endpoint.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 1, 2017

Contributor

Thanks for your comments @aaronlehmann, I'll figure out how to update Swagger (maybe a separate PR for that?)

Ping @vieux @aluzzardi

Contributor

nishanttotla commented Apr 1, 2017

Thanks for your comments @aaronlehmann, I'll figure out how to update Swagger (maybe a separate PR for that?)

Ping @vieux @aluzzardi

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Apr 4, 2017

Collaborator

@nishanttotla sorry to say that, but could you update the swagger in the same PR here is a PR you can use as inspiration: https://github.com/docker/docker/pull/32015/files

Collaborator

vieux commented Apr 4, 2017

@nishanttotla sorry to say that, but could you update the swagger in the same PR here is a PR you can use as inspiration: https://github.com/docker/docker/pull/32015/files

Show outdated Hide outdated api/swagger.yaml Outdated
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Apr 5, 2017

Contributor

@vieux windows test failure seems unrelated.

Contributor

nishanttotla commented Apr 5, 2017

@vieux windows test failure seems unrelated.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Apr 6, 2017

Collaborator

LGTM ping @dnephin

Collaborator

vieux commented Apr 6, 2017

LGTM ping @dnephin

nishanttotla added some commits May 1, 2017

Change GetRepository to take Named arguments
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Adding /distribution/{name}/json endpoint to contact registry
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 9, 2017

Contributor
23:59:35 The swagger spec at "api/swagger.yaml" is invalid against swagger specification 2.0. see errors :
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
Contributor

aaronlehmann commented May 9, 2017

23:59:35 The swagger spec at "api/swagger.yaml" is invalid against swagger specification 2.0. see errors :
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate one and only one schema (oneOf)
23:59:35 - must validate at least one schema (anyOf)
23:59:35 - paths./distribution/{name}/json.get.responses.200.schema.properties.Descriptor.properties.Size.type in body must be of type array: "string"
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 9, 2017

Contributor

@aaronlehmann sorry about that, fixed CI.

Contributor

nishanttotla commented May 9, 2017

@aaronlehmann sorry about that, fixed CI.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla
Contributor

nishanttotla commented May 9, 2017

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 9, 2017

Contributor

Just one comment... it may be a good idea to override the descriptor's media type always, not just in the descriptorFromPayload case, because we know the one the registry give us is going to be misleading. Other than that, it looks good.

Contributor

aaronlehmann commented May 9, 2017

Just one comment... it may be a good idea to override the descriptor's media type always, not just in the descriptorFromPayload case, because we know the one the registry give us is going to be misleading. Other than that, it looks good.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 10, 2017

Contributor

@aaronlehmann sure, updating the PR. If we're going to call Payload() always, then descriptorFromPayload isn't really needed, and we can update size if it's 0.

Contributor

nishanttotla commented May 10, 2017

@aaronlehmann sure, updating the PR. If we're going to call Payload() always, then descriptorFromPayload isn't really needed, and we can update size if it's 0.

@aaronlehmann

Possible cleanup: dscrptr is no longer necessary because distributionInspect.Descriptor can be set directly. Not important though.

LGTM

/distribution/{name}/json returns full Descriptor object
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 10, 2017

Contributor

@aaronlehmann thanks, I cleaned that up.

Contributor

nishanttotla commented May 10, 2017

@aaronlehmann thanks, I cleaned that up.

Features:
type: "array"
items:
type: "string"

This comment has been minimized.

@thaJeztah

thaJeztah May 10, 2017

Member

Perhaps we should add an example response here, because the current output doesn't provide a lot of information to the reader 😄
screen shot 2017-05-10 at 19 13 22

@thaJeztah

thaJeztah May 10, 2017

Member

Perhaps we should add an example response here, because the current output doesn't provide a lot of information to the reader 😄
screen shot 2017-05-10 at 19 13 22

This comment has been minimized.

@nishanttotla

nishanttotla May 10, 2017

Contributor

@thaJeztah updated. PTAL.

screen shot 2017-05-10 at 11 02 24 am

@nishanttotla

nishanttotla May 10, 2017

Contributor

@thaJeztah updated. PTAL.

screen shot 2017-05-10 at 11 02 24 am

This comment has been minimized.

@nishanttotla

nishanttotla May 10, 2017

Contributor

Looks like empty array definitions are still causing an error, I'm fixing those.

@nishanttotla

nishanttotla May 10, 2017

Contributor

Looks like empty array definitions are still causing an error, I'm fixing those.

This comment has been minimized.

@thaJeztah

thaJeztah May 10, 2017

Member

Can we come up with some sample URL's and/or fill the OSVersion / Variants? Or do you think it's ok to keep them empty?

@thaJeztah

thaJeztah May 10, 2017

Member

Can we come up with some sample URL's and/or fill the OSVersion / Variants? Or do you think it's ok to keep them empty?

This comment has been minimized.

@nishanttotla

nishanttotla May 10, 2017

Contributor

@thaJeztah In the examples I saw, these weren't filled, so I think it's okay. I can try to search for better examples later.

@nishanttotla

nishanttotla May 10, 2017

Contributor

@thaJeztah In the examples I saw, these weren't filled, so I think it's okay. I can try to search for better examples later.

type: "string"
Size:
type: "integer"
format: "int64"

This comment has been minimized.

@thaJeztah

thaJeztah May 10, 2017

Member

Wondered if this should be an uint64, but I see it's coming from an existing type

@thaJeztah

thaJeztah May 10, 2017

Member

Wondered if this should be an uint64, but I see it's coming from an existing type

Adding example to /distribution/{name}/json endpoint swagger spec
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla
Contributor

nishanttotla commented May 10, 2017

@thaJeztah PTAL.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 10, 2017

Member

I think We're almost there; left one comment on the example, also (sorry, forgot to mention); this needs an entry in the API history; https://github.com/moby/moby/blob/master/docs/api/version-history.md#v130-api-changes

Member

thaJeztah commented May 10, 2017

I think We're almost there; left one comment on the example, also (sorry, forgot to mention); this needs an entry in the API history; https://github.com/moby/moby/blob/master/docs/api/version-history.md#v130-api-changes

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla May 10, 2017

Contributor

@thaJeztah updated API history here: #33147

Contributor

nishanttotla commented May 10, 2017

@thaJeztah updated API history here: #33147

@mistyhacks

Docs LGTM

@thaJeztah

LGTM, thanks!

@thaJeztah thaJeztah merged commit 28d428f into moby:master May 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33944 has succeeded
Details
janky Jenkins build Docker-PRs 42541 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2910 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13782 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2650 has succeeded
Details

@nishanttotla nishanttotla deleted the nishanttotla:engine-api-manifest branch May 10, 2017

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