Added tags list to /images/:id/json api. #13185

NOX73 · 2015-05-13T13:29:26Z

It closes #10139.

jessfraz · 2015-05-13T22:15:27Z

this will also need docs updates etc. and tests.

jessfraz · 2015-05-13T22:15:56Z

graph/service.go

+			}
+		}
+	}
+	s.Unlock()


this should be on line 36 as defer s.Unlock()

actually not necessary, there is no returns between.

I think structure should be unlocked asap.

If we need a lock at all, can we at least make it a read-lock so we don't block all interactions on the image DB?

Then we have to include sync.RWMutex to TagStore structure instead of sync.Mutex. I could do it. Can this change break something?

Should be ok - but let's try it and see if the tests are ok.

NOX73 · 2015-05-14T09:14:49Z

@jfrazelle should I do these updates here? I couldn't find any tests for this. Could you show me where is it? (Sorry, I'm new to docker contributing)
I find docs here docs/sources/reference/api/docker_remote_api_v1.9.md. Are there any other places which should be updated?

vdemeester · 2015-05-16T08:26:40Z

@NOX73 It should be in the integration test (integration-cli), and because you've change the images/:id/json, I would guess: docker_api_inspect_test.go or docker_api_image_test.go(and maybe in the cli ones, like docker_cli_inspect_tes.go but I doubt it).

After looking a bit it seems there, I think the best place should be in docker_api_inspect_test.go but there is only the container one in there, should create a new one for the image that does almost the same thing.

func (s *DockerSuite) TestInspectApiImageResponse(c *check.C) {
    // […]
    endpoint := "/images/" + imageId + "/json"
    // […]
}

NOX73 · 2015-05-18T13:26:42Z

@vdemeester done, I added test and fixed docs.

runcom · 2015-05-18T13:30:51Z

integration-cli/docker_api_inspect_test.go

+	c.Assert(status, check.Equals, http.StatusOK)
+	c.Assert(err, check.IsNil)
+
+	var inspectJSON map[string]interface{}


use types.ImageInspect instead of map[string]interface{}

vdemeester · 2015-05-25T21:05:23Z

@NOX73 Could you take care of the comment from @runcom ? 😉

NOX73 · 2015-05-26T14:57:38Z

@runcom @vdemeester done

vdemeester · 2015-06-03T18:23:54Z

@NOX73 I think the test needs to be a little more complete, like checking if there the content of the JSON (and in our case, just the fact that there is a Tag field and that it is what we expect)

runcom · 2015-06-03T18:27:40Z

ping @duglin or @calavera about design before moving to code review and rebase, this SGTM

NOX73 · 2015-06-03T19:52:45Z

@vdemeester The fact that JSON correctly unmarshal to ImageInspect type doesn't mean that it's all good? We can check Tags field, but for me it seems just like code duplication. Should I do it?

vdemeester · 2015-06-03T20:02:26Z

@NOX73 hum well you're right, it's a more general test than just checking the tags.. nevermind my comment, the fact it has correctly unmarshal to ImageInspect is ok 😉 😅.

duglin · 2015-06-03T21:33:00Z

integration-cli/docker_api_inspect_test.go

+		c.Fatalf("failed to pull image: %s, %v", out, err)
+	}
+
+	runCmd = exec.Command(dockerBinary, "images", "-q", "busybox")


Up to you if you want to change it, but you don't need to get the ID for busybox, you can just do GET /images/busybox/json down below.

@duglin is it pulled somewhere before? I don't see.

All I meant is that they could use 'busybox' (by name) instead of busybox's ID. Should result in the same thing, right?

Oh, yes. I get it. Thanks, I'll do it.

@duglin done

runcom · 2015-06-03T21:41:41Z

integration-cli/docker_api_inspect_test.go

+	status, body, err := sockRequest("GET", endpoint, nil)
+
+	c.Assert(status, check.Equals, http.StatusOK)
+	c.Assert(err, check.IsNil)


check error before status pls

@runcom done

thaJeztah · 2015-06-15T15:26:55Z

@NOX73 looks like this needs to be rebased as well. Also (but we're not in docs-review yet), the changes to the API documentation need to be moved to docker_remote_api_v1.20.md

cpuguy83 · 2015-08-12T13:09:03Z

integration-cli/docker_api_inspect_test.go

 	"github.com/go-check/check"
 )

+func contains(tags []string, tag string) bool {


A function like this exists here:
https://github.com/docker/docker/blob/master/pkg/stringutils/stringutils.go#L45

It's slightly different because it's is discarding case but I don't think that matters here. If so, let's use that.

vdemeester · 2015-08-15T21:15:18Z

ping @NOX73, could you take care of @cpuguy83 comments ? 😉

NOX73 · 2015-08-16T20:35:07Z

yes, of course, asap, give me one or two days

NOX73 · 2015-08-21T13:25:37Z

@vdemeester @cpuguy83 could you take a look here )

cpuguy83 · 2015-08-22T01:18:25Z

docs/reference/api/docker_remote_api_v1.21.md

-         "Id": "b750fe79269d2ec9a3c593ef05b4332b1d1a02a62b4accb2c21d589ff2f5f2dc",
-         "Parent": "27cf784147099545",
-         "Size": 6824592
+      "ContainerConfig" : {


Something happened here.
Was this wrong in the old version?

@cpuguy83 ohh yeah, it misses some field (Config object for example).
Although I don't have the same output as @NOX73 :

{ "Id": "91e54dfb11794fad694460162bf0cb0a4fa710cfa3f60979c177d920813e267c", "Parent": "d74508fb6632491cea586a1fd7d748dfc5274cd6fdfedee309ecdcbc2bf5cb82", "Comment": "", "Created": "2015-08-20T20:21:15.767240511Z", "Container": "74bb7db8d212f77ab6d467b710451e54d2c60533f641de8c91e7ef343b88a146", "ContainerConfig": { "Hostname": "e611e15f9c9d", "Domainname": "", "User": "", "AttachStdin": false, "AttachStdout": false, "AttachStderr": false, "ExposedPorts": null, "PublishService": "", "Tty": false, "OpenStdin": false, "StdinOnce": false, "Env": null, "Cmd": [ "\/bin\/sh", "-c", "#(nop) CMD [\"\/bin\/bash\"]" ], "Image": "d74508fb6632491cea586a1fd7d748dfc5274cd6fdfedee309ecdcbc2bf5cb82", "Volumes": null, "VolumeDriver": "", "WorkingDir": "", "Entrypoint": null, "NetworkDisabled": false, "MacAddress": "", "OnBuild": null, "Labels": {} }, "DockerVersion": "1.7.1", "Author": "", "Config": { "Hostname": "e611e15f9c9d", "Domainname": "", "User": "", "AttachStdin": false, "AttachStdout": false, "AttachStderr": false, "ExposedPorts": null, "PublishService": "", "Tty": false, "OpenStdin": false, "StdinOnce": false, "Env": null, "Cmd": [ "\/bin\/bash" ], "Image": "d74508fb6632491cea586a1fd7d748dfc5274cd6fdfedee309ecdcbc2bf5cb82", "Volumes": null, "VolumeDriver": "", "WorkingDir": "", "Entrypoint": null, "NetworkDisabled": false, "MacAddress": "", "OnBuild": null, "Labels": {} }, "Architecture": "amd64", "Os": "linux", "Tags" : [ "ubuntu:14.04", "ubuntu:latest" ], "Size": 0, "VirtualSize": 188333286, "GraphDriver": { "Name": "btrfs", "Data": null } }

I think we need to create some script to start a container producing the right output for updating these examples; for example, the "labels" are now missing in the output, which I think we want to keep in there.

(I know I commented on another PR with an example docker run to include the labels. I can look it up if you want)

I think we need to create some script to start a container producing the right output for updating these examples

Definitely 👍

for example, the "labels" are now missing in the output, which I think we want to keep in there.

Not sure I got this one.

The old example output had some example labels in the output;
#13185 (diff), that got lost in the new example output;
https://github.com/docker/docker/pull/13185/files#diff-3acbcaa129d9a6972e140d1b4c32cc27R1433

ohh right ! I see 😅

That's why I think we could use a simple script that builds images and containers, using the same settings each time, and curls the API, capturing the output for copy/paste in the docs (or something similar) 😄

This is likely because the image was created with a different docker version.

So, should I update this docs here?

@NOX73 sorry for my late response; yes, if you can update the docs and make sure the labels are in there, that would be great!

cpuguy83 · 2015-08-24T14:25:43Z

LGTM

bfirsh · 2015-08-28T14:21:39Z

@NOX73 I'm afraid this needs a rebase again - sorry! If you could fix the image output in the docs at the same time, that'd be grand.

Thanks for your patience!

NOX73 · 2015-08-30T22:42:29Z

@bfirsh @cpuguy83 rebased & updated /image/ubuntu/json example json in docs.

calavera · 2015-09-05T00:54:11Z

LGTM. Moving to docs review

SvenDowideit · 2015-09-05T12:27:31Z

docs LGTM - @thaJeztah @moxiegirl

thaJeztah · 2015-09-05T12:43:51Z

docs/reference/api/docker_remote_api_v1.21.md

+        "WorkingDir" : "",
+        "VolumeDriver" : "",
+        "OnBuild" : null,
+        "Labels" : {},


Looks like the labels got lost in the example output; could you make sure the example output contains some labels?

See my earlier comment for some instructions to create images with the labels that were present previously; #13185 (comment)

@NOX73 can you do this and rebase? We'll merge as soon as everything is green again.

@calavera done, waiting for checks ...

It closes moby#10139. Signed-off-by: Rozhnov Alexandr <nox73@ya.ru>

thaJeztah · 2015-09-10T23:32:17Z

docs LGTM

waiting for Janky/Windows

Added tags list to /images/:id/json api.

GordonTheTurtle added status/0-triage dco/no labels May 13, 2015

NOX73 force-pushed the 10139-tags-list-get-image-api branch from 76cef41 to 943c63b Compare May 13, 2015 13:36

GordonTheTurtle removed the dco/no label May 13, 2015

jessfraz added status/1-design-review and removed status/0-triage labels May 13, 2015

jessfraz reviewed May 13, 2015
View reviewed changes

NOX73 force-pushed the 10139-tags-list-get-image-api branch from 943c63b to f3c764e Compare May 18, 2015 13:23

runcom reviewed May 18, 2015
View reviewed changes

NOX73 force-pushed the 10139-tags-list-get-image-api branch from f3c764e to 5e5cab6 Compare May 26, 2015 14:57

NOX73 force-pushed the 10139-tags-list-get-image-api branch from 5e5cab6 to 14d35b8 Compare June 2, 2015 09:35

duglin reviewed Jun 3, 2015
View reviewed changes

duglin added status/2-code-review and removed status/1-design-review labels Jun 3, 2015

runcom reviewed Jun 3, 2015
View reviewed changes

NOX73 force-pushed the 10139-tags-list-get-image-api branch 2 times, most recently from 3f37678 to c27eac0 Compare June 15, 2015 15:15

NOX73 force-pushed the 10139-tags-list-get-image-api branch from c27eac0 to 8a41211 Compare June 18, 2015 20:49

cpuguy83 reviewed Aug 12, 2015
View reviewed changes

NOX73 force-pushed the 10139-tags-list-get-image-api branch 2 times, most recently from 251c6fa to 401e372 Compare August 21, 2015 11:03

cpuguy83 reviewed Aug 22, 2015
View reviewed changes

vdemeester mentioned this pull request Aug 29, 2015

docker inspect image-id: why are there no details about repository(s) and tag(s)? #15930

Closed

NOX73 force-pushed the 10139-tags-list-get-image-api branch from 401e372 to a19b7f7 Compare August 30, 2015 22:39

calavera added status/3-docs-review and removed status/2-code-review labels Sep 5, 2015

thaJeztah reviewed Sep 5, 2015
View reviewed changes

NOX73 force-pushed the 10139-tags-list-get-image-api branch from a19b7f7 to d80ce7e Compare September 10, 2015 08:20

Added tags list to /images/:id/json api.

e9e68fa

It closes moby#10139. Signed-off-by: Rozhnov Alexandr <nox73@ya.ru>

NOX73 force-pushed the 10139-tags-list-get-image-api branch from d80ce7e to e9e68fa Compare September 10, 2015 08:55

thaJeztah added status/4-merge and removed status/3-docs-review labels Sep 10, 2015

calavera added a commit that referenced this pull request Sep 10, 2015

Merge pull request #13185 from NOX73/10139-tags-list-get-image-api

bb1996a

Added tags list to /images/:id/json api.

calavera merged commit bb1996a into moby:master Sep 10, 2015

thaJeztah added impact/api impact/changelog labels Sep 11, 2015

tonistiigi mentioned this pull request Oct 21, 2015

Tags in docker inspect contains digests #17246

Closed

thaJeztah added this to the 1.9.0 milestone Oct 28, 2015

thaJeztah mentioned this pull request Nov 4, 2015

Report sha256 digest in docker inspect #17670

Closed

Added tags list to /images/:id/json api. #13185

Added tags list to /images/:id/json api. #13185

Conversation

NOX73 commented May 13, 2015

jessfraz commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NOX73 commented May 14, 2015

vdemeester commented May 16, 2015

NOX73 commented May 18, 2015

Choose a reason for hiding this comment

vdemeester commented May 25, 2015

NOX73 commented May 26, 2015

vdemeester commented Jun 3, 2015

runcom commented Jun 3, 2015

NOX73 commented Jun 3, 2015

vdemeester commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jun 15, 2015

Choose a reason for hiding this comment

vdemeester commented Aug 15, 2015

NOX73 commented Aug 16, 2015

NOX73 commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 commented Aug 24, 2015

bfirsh commented Aug 28, 2015

NOX73 commented Aug 30, 2015

calavera commented Sep 5, 2015

SvenDowideit commented Sep 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Sep 10, 2015