Update engine api with required arguments by vdemeester · Pull Request #21657 · moby/moby

vdemeester · 2016-03-30T16:12:02Z

This PR is related to docker/engine-ap#162 and is it's inclusion in docker/docker. It's still a little bit a work in progress but I wanted to have both PR opened at the same time to make sure it's ok for everybody and to measure the impact of the change on the code here 👼.

The docker-archive-public/docker.engine-api#162 (docker-archive-public/docker.engine-api#137 for the related issue) changes some client signature to add the required arguments in the signature (to make it easier to use by telling the consumer what is required and what is not) 🐯.

It also tries to remove the image/tag distinction from the client side for now, and replace it with the reference instead.
This has some impact on the content trust code (api/client/trust.go).

This is also a very small step to remove the reference package (or at least reduce the usage) from this repository (and use docker/distribution/reference in the future, once distribution/distribution#1277 is merged) 🐳.

/cc @calavera @stevvooe @MHBauer @dmcgowan @aaronlehmann @tonistiigi @LK4D4

It's a little bit a chicken-and-egg problem, as depending on the review here, the code in the API will have to be updated. Anyway, we have to wait for docker-archive-public/docker.engine-api#162 to get merged first but we will merge it once it's green, reviewed and OK here 👼 .

Let's hope it's green first 😅.

🐸

vdemeester · 2016-03-30T16:15:31Z

This message is there since at least 1.8, so I think it could be removed. If yes, then the tag variable is no more needed and the Tag attribute in ImageImportOptions neither.

thaJeztah · 2016-03-30T16:17:41Z

Looks like CI is failing:

16:15:28 + go test -check.f DockerSuite -check.v -check.timeout=90m -test.timeout=360m github.com/docker/docker/integration-cli
16:15:57 # github.com/docker/docker/integration-cli
16:15:57 ./docker_api_network_test.go:32: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:47: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:51: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:103: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:129: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:173: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:186: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:199: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:212: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:214: unknown "github.com/docker/engine-api/types".NetworkCreate field 'Name' in struct literal
16:15:57 ./docker_api_network_test.go:214: too many errors

aaronlehmann · 2016-03-30T17:22:27Z

Why is this removed?

oh you are right, something is missing in the ImagePush in remote API (the same check done in ImageTag in fact)

Is this resolved?

Looks like it was resolved https://github.com/docker/engine-api/pull/162/files#diff-af7c2f642033c3058dc4c26810d875a4R27

aaronlehmann · 2016-03-30T17:32:32Z

I like the direction this is going. My main question is about references being passed as strings into engine-api functions. Would it be better to use the actual distribution/reference types? I see cases where we're taking a parsed reference and calling String before passing it to an API function, and then the API function parses it again. I also see arguments like repoInfo.Name()+"@"+r.digest.String().

I think there are advantages to passing in actual references to the API functions. It prevents you from making mistakes like passing in a tag where you're really supposed to pass a fully-qualified reference.

Are there reasons not to expose these types at the API level? Or do we plan to do that, with this PR as an intermediate step on the way there?

@stevvooe WDYT?

stevvooe · 2016-03-30T20:42:46Z

@aaronlehmann I'm not sure that the distribution/reference types are ready for prime time. We are still seeing issues with their use in docker/docker. While they provide correctness, they are slightly clunky for common API use. I'd rather people just take a string user input and shove it into this function, where we can validate and parse it correctly.

I think there are advantages to passing in actual references to the API functions. It prevents you from making mistakes like passing in a tag where you're really supposed to pass a fully-qualified reference.

Are there reasons not to expose these types at the API level? Or do we plan to do that, with this PR as an intermediate step on the way there?

I agree 100% with the sentiment here but we may want to consider this PR to be an intermediate step.

Ideally, these all become completely server-side, avoiding the need to parse and validate them client-side.

vdemeester · 2016-03-31T10:24:33Z

Are there reasons not to expose these types at the API level? Or do we plan to do that, with this PR as an intermediate step on the way there?

I agree with @stevvooe, "Ideally, these all become completely server-side, avoiding the need to parse and validate them client-side.". It make it simpler for a user of the engine-api to use it (like in libcompose) and it's kinda consistent with the cli (in the way that you specify the image reference as a "string" on its own on the cli, should be the same using the api).

And yes, I see this PR as a first step on moving towards having the reference parsing handling server-side (but it's a little more work and it would make the PR bigger).

vdemeester · 2016-03-31T15:15:14Z

Updated the code (here and in the engine-api PR) and fixed the build 😸

calavera · 2016-04-12T19:07:48Z

@vdemeester the change in engine-api was merged this morning. Feel free to update this PR. Moving to code review.

vdemeester · 2016-04-13T11:14:05Z

Updated.

Win2Lin is green :

11:15:29 23cd6eeee6e6
11:15:39 INFO: Cleanup complete
11:15:39 Notifying endpoint 'HTTP:https://leeroy.dockerproject.org/notification/jenkins'
11:15:39 Finished: SUCCESS

Userns is not 😓

11:11:50 ----------------------------------------------------------------------
11:11:50 FAIL: docker_cli_run_unix_test.go:808: DockerSuite.TestRunSeccompProfileDenyUnshareUserns
11:11:50 
11:11:50 docker_cli_run_unix_test.go:839:
11:11:50     c.Fatalf("expected unshare userns with seccomp profile denied to fail, got %s", out)
11:11:50 ... Error: expected unshare userns with seccomp profile denied to fail, got /go/src/github.com/docker/docker/bundles/1.11.0-dev/test-integration-cli/../binary/docker: Error response from daemon: Untar re-exec error: exit status 1: output: no such file or directory.
11:11:50 See '/go/src/github.com/docker/docker/bundles/1.11.0-dev/test-integration-cli/../binary/docker run --help'.
11:11:50 
11:11:50 
11:11:50 
11:11:50 ----------------------------------------------------------------------

calavera · 2016-04-13T18:11:59Z

LGTM

stevvooe · 2016-04-13T18:20:02Z

Do we not have a way to assemble canonical references? https://godoc.org/github.com/docker/distribution/reference#WithDigest

May have to use the docker/docker/reference version.

Hum you're right

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Using new methods from engine-api, that make it clearer which element is required when consuming the API. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2016-04-15T15:40:26Z

@stevvooe updated (using WithDigest from docker/docker/reference for now – as it's already used elsewhere in this file)

stevvooe · 2016-04-15T19:39:05Z

LGTM

GordonTheTurtle added the status/0-triage label Mar 30, 2016

vdemeester reviewed Mar 30, 2016
View reviewed changes

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 30, 2016

vdemeester force-pushed the update-engine-api branch 2 times, most recently from 0f4441d to 27a8c82 Compare March 30, 2016 16:45

aaronlehmann reviewed Mar 30, 2016
View reviewed changes

aaronlehmann added status/1-design-review and removed status/0-triage labels Mar 30, 2016

vdemeester force-pushed the update-engine-api branch 2 times, most recently from 46ae5f2 to 2db3c07 Compare March 31, 2016 12:48

aaronlehmann removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 31, 2016

vdemeester mentioned this pull request Apr 7, 2016

Remove reference package dependency from the api. #21867

Merged

vdemeester force-pushed the update-engine-api branch 3 times, most recently from 04c3702 to bc1658c Compare April 12, 2016 15:48

calavera added status/2-code-review and removed status/1-design-review labels Apr 12, 2016

vdemeester force-pushed the update-engine-api branch from bc1658c to 3cfbda8 Compare April 13, 2016 08:37

vdemeester changed the title ~~WIP: Update engine api with required arguments~~ Update engine api with required arguments Apr 13, 2016

stevvooe reviewed Apr 13, 2016
View reviewed changes

Vendor engine-api with required arguments

9802d7d

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Update client code with api changes

b9c94b7

Using new methods from engine-api, that make it clearer which element is required when consuming the API. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the update-engine-api branch from 3cfbda8 to b9c94b7 Compare April 15, 2016 10:48

calavera merged commit 7fd53f7 into moby:master Apr 19, 2016

vdemeester deleted the update-engine-api branch April 19, 2016 07:53

tonistiigi mentioned this pull request Apr 21, 2016

Implicit pull on run broken on master #22240

Closed

Conversation

vdemeester commented Mar 30, 2016

Uh oh!

vdemeester Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 30, 2016

Uh oh!

aaronlehmann Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

dmcgowan Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 30, 2016

Uh oh!

stevvooe commented Mar 30, 2016

Uh oh!

vdemeester commented Mar 31, 2016

Uh oh!

vdemeester commented Mar 31, 2016

Uh oh!

calavera commented Apr 12, 2016

Uh oh!

vdemeester commented Apr 13, 2016

Uh oh!

calavera commented Apr 13, 2016

Uh oh!

stevvooe Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Apr 15, 2016

Uh oh!

stevvooe commented Apr 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants