Make required arguments required. by vdemeester · Pull Request #162 · docker-archive-public/docker.engine-api

vdemeester · 2016-03-20T11:31:23Z

Change method signatures to make required arguments actually required (using a paramater), all accross the API 🐮.

It's not complete (there is a few FIXME still), I'm opening the PR to gather opinion on this ; I also think it should not be merged until docker 1.11 is released 🐵.

@stevvooe is this something that you had in mind ?

/cc @calavera @MHBauer @dongluochen

This is related to and fixes #137.

🐸

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

stevvooe · 2016-03-21T20:59:59Z

-	ImageID      string // ImageID is the name of the image to pull
-	Tag          string // Tag is the name of the tag to be pulled
-	RegistryAuth string // RegistryAuth is the base64 encoded credentials for the registry
+	Tag           string // Tag is the name of the tag to be pulled


Tag should be part of argument. For example, "redis:latest" should be part of the argument. For now, we have to parse this on the client side, but in the future, this format should be daemon specific.

stevvooe · 2016-03-21T21:12:41Z

@vdemeester This is exactly what I was talking about. This is a fantastic start!

MHBauer · 2016-03-21T21:26:13Z

I like it. Makes using the api more understandable at a glance.

calavera · 2016-03-23T21:33:05Z

go for it!

I also think it should not be merged until docker 1.11 is released

agreed

calavera · 2016-03-25T18:14:08Z

@@ -0,0 +1,27 @@
+package client


should this be in types?

Should we have a more general package under types/reference? We've talked about this several times, maybe now it's the right moment to do it

@calavera ah yes, good idea 😉

vdemeester · 2016-03-27T19:58:21Z

@stevvooe updated with reference handling. I'm not sure I got all right but 😝

calavera · 2016-03-28T23:25:47Z

LGTM

stevvooe · 2016-03-29T20:03:12Z

+		tag = tagged.Tag()
+	}
+	// This is in order to support a reference like :
+	// tag@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff


Calling this tag is confusing. Use name. When speaking about image references, the tag is the portion after the name (ie name:tag or name:tag@digest).

Ah I see what you mean :)

Change method signatures to make required arguments actually required (using a paramater), all accross the API. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

At some point the format of the reference should be daemon specific, and thus should not be *imposed* via the repository:tag current arguments. The engine-api library should encorage the use of reference. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2016-03-30T12:18:43Z

-	ContainerAttach(ctx context.Context, options types.ContainerAttachOptions) (types.HijackedResponse, error)
-	ContainerCommit(ctx context.Context, options types.ContainerCommitOptions) (types.ContainerCommitResponse, error)
+	ContainerAttach(ctx context.Context, container string, options types.ContainerAttachOptions) (types.HijackedResponse, error)
+	ContainerCommit(ctx context.Context, container string, options types.ContainerCommitOptions) (types.ContainerCommitResponse, error)


Hum, there is RepositoryName and Tag in the types.ContainerCommitOptions struct.. It could also be replaced by a ref, I'll look into it.

aaronlehmann · 2016-03-30T17:18:48Z

+	// This is in order to support a reference like :
+	// name@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
+	if digested, isDigested := ref.(distreference.Digested); isDigested {
+		tag = fmt.Sprintf("%s@%s", tag, digested.Digest().String())


I don't think the Docker API currently supports providing both a tag and a digest. Do you know of any places where this is allowed?

BTW, if we keep this, I would drop the Sprintf and do tag += "@" + digested.Digest().String().

I would prefer not to introduce another string format which is a subset of the reference format. If we want to support both tag and digest in the Docker API we should send it as a full reference and not try and to sneak it in with the tag.

The problem here is the variable naming. The name tag should be name. The code may be wrong, as well.

I think you are right, there is a disconnect between what the comment says it is supporting name@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which would assume a valid reference such as docker.io/library/ubuntu@name@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff and what the code is doing latest@name@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which is not a valid reference.

oh that's right, I completely messed up this part, working on it 👼

… for reference related code O:). Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2016-04-04T08:20:11Z

@dmcgowan @aaronlehmann @stevvooe updated 👼

stevvooe · 2016-04-04T19:04:13Z

LGTM

I think we should put a note there that we intend to fix the fact that digest is transmitted in tag. That was not the intention. We should probably have an issue somewhere to simply take a ref over the API.

vdemeester · 2016-04-05T13:59:43Z

@stevvooe You mean for https://github.com/docker/engine-api/pull/162/files#diff-dbeb877b7334f75bc4c9063000523d71R19 or on a more general scale ? (every part of the Image* api that has repository and tag ?)

Once this (and the docker/docker counter part) is merged, I'll create an issue about it and start to work this out in both repository 😉

stevvooe · 2016-04-05T18:36:02Z

@vdemeester I mean on a more general scale, but I am not sure about the level of complexity. The client-side parsing introduces a lot of errors but the reduced complexity may make the API easier to use in the future.

vdemeester · 2016-04-08T11:37:35Z

/cc @calavera still LGTM ? 👼

vdemeester · 2016-04-09T11:49:13Z

/cc @MHBauer @dongluochen @vieux @duglin for final reviews too 👼

calavera · 2016-04-11T22:30:30Z

LGTM

dongluochen · 2016-04-11T22:43:26Z

 // ContainerLogs returns the logs generated by a container in an io.ReadCloser.
 // It's up to the caller to close the stream.
-func (cli *Client) ContainerLogs(ctx context.Context, options types.ContainerLogsOptions) (io.ReadCloser, error) {
+func (cli *Client) ContainerLogs(ctx context.Context, container string, options types.ContainerLogsOptions) (io.ReadCloser, error) {


I suggest updating container to containerID? There are other places with this problem. No ambiguity is better.

These can take a name (I think), so containerID is inaccurate.

@dongluochen I used container on purpose as it can be container ID or name (also did the change elsewhere).

agreed, container is a better name.

dongluochen · 2016-04-11T23:15:09Z

LGTM. I have a small change quest on parameter names.

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

vdemeester added status/1-design-review and removed status/0-triage labels Mar 20, 2016

calavera added the status/wait-until-release-freeze label Mar 21, 2016

stevvooe reviewed Mar 21, 2016
View reviewed changes

vdemeester changed the title ~~WIP: Make required arguments required.~~ Make required arguments required. Mar 25, 2016

calavera added status/2-code-review and removed status/1-design-review status/wait-until-release-freeze labels Mar 25, 2016

calavera reviewed Mar 25, 2016
View reviewed changes

stevvooe reviewed Mar 29, 2016
View reviewed changes

vdemeester added 2 commits March 30, 2016 12:33

Make required arguments required.

486171b

Change method signatures to make required arguments actually required (using a paramater), all accross the API. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester reviewed Mar 30, 2016
View reviewed changes

vdemeester mentioned this pull request Mar 30, 2016

Update engine api with required arguments moby/moby#21657

Merged

aaronlehmann reviewed Mar 30, 2016
View reviewed changes

Creates a types/reference package…

acf8a70

… for reference related code O:). Signed-off-by: Vincent Demeester <vincent@sbr.pm>

Random-Liu mentioned this pull request Apr 7, 2016

Refactor image related functions to use docker engine-api kubernetes/kubernetes#23800

Merged

vdemeester mentioned this pull request Apr 7, 2016

Remove reference package dependency from the api. moby/moby#21867

Merged

dongluochen reviewed Apr 11, 2016
View reviewed changes

calavera merged commit a6dca65 into docker-archive-public:master Apr 12, 2016

vdemeester deleted the 137-required-containers branch April 12, 2016 20:46

nishanttotla mentioned this pull request Apr 13, 2016

go get github.com/docker/swarm broken docker-archive/classicswarm#2111

Closed

vdemeester mentioned this pull request Apr 13, 2016

Update engine api with required docker-archive-public/docker.libcompose#201

Merged

Random-Liu mentioned this pull request Apr 14, 2016

Kubelet: Refactor container related functions in DockerInterface kubernetes/kubernetes#23699

Merged

nishanttotla mentioned this pull request May 19, 2016

Update engine-api version in /vendor docker-archive/classicswarm#2253

Closed

cpuguy83 mentioned this pull request Jun 29, 2016

1.12-rc2 undocumented change in exec remote API moby/moby#24055

Closed

Conversation

vdemeester commented Mar 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Mar 21, 2016

Uh oh!

MHBauer commented Mar 21, 2016

Uh oh!

calavera commented Mar 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Mar 27, 2016

Uh oh!

calavera commented Mar 28, 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!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Apr 4, 2016

Uh oh!

stevvooe commented Apr 4, 2016

Uh oh!

vdemeester commented Apr 5, 2016

Uh oh!

stevvooe commented Apr 5, 2016

Uh oh!

vdemeester commented Apr 8, 2016

Uh oh!

vdemeester commented Apr 9, 2016

Uh oh!

calavera commented Apr 11, 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!

dongluochen commented Apr 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants