pull client API version negotiation out of the CLI and into the client #32779

rremer · 2017-04-22T19:01:01Z

- What I did

Added some tests for client version discovery, and moved that logic out of the CLI and into the client.

The drive here is that I was using the go docker client, and had to call UpdateVersion with a string I needed to get from the server. I noticed that the CLI never had to do this, and sure enough there was some server version negotiation going on in the CLI. At this commit, the CLI just implements a normal client, and the client owns the logic for server version negotiation (so folks consuming the client library can enjoy that same functionality).

- How I did it

Added a new conditional statement in client/client.go UpdateClientVersion(str). If the string is empty, it will trigger DowngradeClientVersion() and step into some cut/paste logic that already existed in cli/command/cli.go for pinging the server, grabbing the APIVersion string, and updating that.

- How to verify it

docker run --privileged -e DOCKER_GITCOMMIT=eebfdd0 docker hack/make.sh test-unit test-integration-cli test-docker-py

or

cd client
go test

The manual/functional verification for the CLI built against this commit:

start a daemon of an older version
docker info

The test cases cover a few more edges than that:

if the server doesn't return an APIVersion
if the environment of the CLI specifically pins DOCKER_API_VERSION

- Description for the changelog

UpdateClientVersion can auto-downgrade to the server APIVersion

- A picture of a cute animal (not mandatory but encouraged)

vdemeester · 2017-04-26T13:20:52Z

@rremer build error

09:50:33 --- FAIL: TestClientDebugEnabled (0.00s)
09:50:33 	assertions.go:226: 
                          
	Error Trace:	docker_test.go:21
09:50:33 		
	Error:      	Received unexpected error:
09:50:33 		
	            	Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
09:50:33 FAIL
09:50:33 coverage: 29.1% of statements

dnephin

Overall I think this is good, so design LGTM

dnephin · 2017-04-26T14:41:00Z

cli/command/cli.go

+	ping, err := cli.client.Ping(context.Background())
+	if err != nil {
+		return err
+	}


We don't want to fail here if there is an error. A user might just be running with --help to figure out how to specify the docker host.

This should return nil, and I guess could use a comment about why.

dnephin · 2017-04-26T14:48:19Z

client/client.go

+	// If no version specified, negotiate with the server
+	if v == "" {
+		cli.DowngradeClientVersion()
+	}


There are two distinct use cases for this function so I don't think it makes sense to group these together.

DockerCLI.Initialize() should call DowngradeClientVersionPing() directly, and leave this functions as it was.

We might be able to just delete UpdateClientVersion() , I don't think it's called from anywhere else.

Indeed there are no other references to UpdateClientVersion that I can find, however removing it would:

potentially break existing consumers of the client library

put update logic into the new negotiate function (although, it is just updating a field)

We don't make any promises about not breaking the API on client/ it's not being released as a versioned package.

dnephin · 2017-04-26T14:53:23Z

client/client.go

+func (cli *Client) DowngradeClientVersion() {
+	ping, _ := cli.Ping(context.Background())
+	cli.DowngradeClientVersionPing(ping)
+}


I think this needs a more appropriate name, It's not necessarily downgrading, it might just be verifying the version is correct. It's also the "APIVersion", not the client version. Maybe NegotiateAPIVersion() ?

dnephin · 2017-04-26T15:09:54Z

client/client.go

+
+// DowngradeClientVersionPing downgrades the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) DowngradeClientVersionPing(p types.Ping) {


Same comment about the function name, how about UpdateAPIVersionFromPing() ?

The check for if !cli.manualOverride {...} needs to move here. It should be the first line in the function I think:

if cli.manualOverride { return }

dnephin · 2017-04-26T15:13:13Z

client/client_test.go

+
+	// test downgrade
+	client.DowngradeClientVersionPing(ping)
+	if client.version != expected {


assert.Equal(t, expected, client.version)

(same with the other assertions)

I was trying to use the same format as the rest of the tests in this suite. If I understood your suggestion, we want to use an assertion framework, so I reworked the tests here to use the same assertion library found in other packages here. I kept this in a separate commit, since it's unrelated cleanup.

Yes, we are slowly migrating everything to testify assertions.

rremer · 2017-04-28T01:51:04Z

Thanks for the feedback, @dnephin !

dnephin · 2017-04-28T19:28:57Z

cli/command/cli.go

@@ -171,6 +170,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {

 			opts.Common.TLSOptions.Passphrase = passwd
 			cli.client, err = NewAPIClientFromFlags(opts.Common, cli.configFile)
+			cli.client.NegotiateAPIVersion()


This doesn't seem like the right place for this. We also don't want to call ping again. I think this line needs to be removed.

Ah yes, I misunderstood the request to move this to "Initialize". I'm guessing we meant "When we remove UpdateClientVersion, then NegotiateAPIVersion should just take its place".

dnephin · 2017-04-28T19:29:15Z

cli/command/cli.go

 	}

+	cli.client.UpdateClientVersion("")


This should be `NegotiateAPIVersionPing(ping)

dnephin

LGTM

cpuguy83 · 2017-06-09T01:48:41Z

This one needs a rebase because the CLI has been removed from this repo (though the client still exists here).

rremer · 2017-06-13T01:28:55Z

Woah, I leave the country for a bit and the whole project disappears! I've rebased, and once it's merged/released we can update the cli to take advantage of NegotiateAPIVersion() @cpuguy83

Incidentally, since the project restructure, moby/moby:master has a failing test in client_test.go: TestClientRedirect, happy to investigate that, just wanted to point out that it's unrelated to this change.

dnephin · 2017-06-13T17:45:49Z

client/client.go

+// NegotiateAPIVersion updates the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) NegotiateAPIVersion() {
+	ping, _ := cli.Ping(context.Background())


I guess context should be an argument to this function. the caller can decide if they have a context they'd like to re-use, or if they want to start a new background context.

dnephin

LGTM

rremer · 2017-06-16T03:22:47Z

It appears that node azure-windows-rs1-5 may require daemon/logger/adapter_test.go:137 to be more than 10 milliseconds. Slow shared tenant windows vms aside, we probably don't want to consider log reading to be a failure if it takes more than 10 milliseconds . Maybe >1s is a failure?

dnephin

LGTM

And yes, that timeout should be increased. I've seen it fail a few times on windowsRS1

rremer · 2017-06-19T01:59:59Z

FYI, I standardized the timeouts in daemon logger tests in #33727, and implemented the same assertion framework there @dnephin ... so hopefully you won't have to rebuild as much :p

cpuguy83 · 2017-06-20T16:55:59Z

client/client_test.go

+}
+
+// MapToEnv takes a map of environment variables and sets them
+func MapToEnv(env map[string]string) {


This should not be exported.

cpuguy83 · 2017-06-20T16:56:03Z

client/client_test.go

 	}
 }

+// EnvToMap returns a map of environment variables
+func EnvToMap() map[string]string {


This should not be exported.

sure, done.

Signed-off-by: Royce Remer <royceremer@gmail.com>

cpuguy83 · 2017-06-21T14:12:56Z

client/client.go

+
+// NegotiateAPIVersionPing updates the version string associated with this
+// instance of the Client to match the latest version the server supports
+func (cli *Client) NegotiateAPIVersionPing(p types.Ping) {


Does this need to be exposed on the interface?

I believe it does. This is what we'll call from docker/cli because we already have a Ping object, which we need to store HasExperimental and OSType.

The other one is a "convenience" for other consumers of this library.

cpuguy83

LGTM

Squashed commit of the following: commit ed225df Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:29:39 2017 +0900 Fix CI commit 94a625d Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:26:49 2017 +0900 Fix windows installer commit 0bea8fb Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:26:20 2017 +0900 Fix README for Windows installer commit 237a4a7 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 19:11:19 2017 +0900 Fix build for Windows commit bc508d7 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:59:16 2017 +0900 Fix to set API version (moby/moby#32779) commit 442be2f Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:03:01 2017 +0900 Update README for Windows commit e83b2ed Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:01:01 2017 +0900 Add installer for Windows commit 776b931 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:00:38 2017 +0900 Build for Windows commit bc01aef Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:43:43 2017 +0900 Fix `run.go` for Windows commit 63b9490 Merge: a72237f 0fea7e2 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:43:01 2017 +0900 Merge tag '0.1.0' of https://github.com/bfirsh/whalebrew into support-windows commit a72237f Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:26:29 2017 +0900 Fix `ForceInstall` for windows commit de458d9 Merge: 03d00bf e7d0792 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:22:41 2017 +0900 Merge tag '0.0.5' into support-windows commit 03d00bf Author: 3846masa <3846masahiro+git@gmail.com> Date: Tue Feb 14 00:01:52 2017 +0900 Fix batch file's shebang commit 21bcf14 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 22:19:25 2017 +0900 Fix .travis.yml for forked project commit a0e486c Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:59:19 2017 +0900 Add appveyor.yml commit fe06f35 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:34:46 2017 +0900 MakePackagePath commit bc12aa0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:23:31 2017 +0900 Rewrite test commit 7453d37 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 20:28:33 2017 +0900 `path.Join` -> `filepath.Join` commit 4ded1a2 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:53:51 2017 +0900 LF -> CRLF in batch file commit 8078165 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:34:08 2017 +0900 Fix edit for Windows commit 8746496 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:15:55 2017 +0900 Fix run for windows commit bb652d4 Merge: 8a6d783 e7e6427 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:08:42 2017 +0900 Merge v0.0.4 Merge remote-tracking branch 'origin/master' into support-windows commit 8a6d783 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 21:11:59 2017 +0900 Support to run and return status code on Windows commit ce3d298 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:58:21 2017 +0900 Revert "syscall -> exec (for working on windows)" This reverts commit 276a9cd. commit ffbdd15 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:50:08 2017 +0900 Add batch file support (for working on windows) commit cdc5a05 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:34:30 2017 +0900 Revert "Add batch file support (for working on windows)" This reverts commit aba79b0. commit 10e10a0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Feb 3 00:53:23 2017 +0900 Revert "Fix checking args (for working on windows)" This reverts commit 926f422. commit 74db526 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 03:14:59 2017 +0900 Fix default WHALEBREW_INSTALL_PATH (for working on windows) commit aba79b0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 03:06:00 2017 +0900 Add batch file support (for working on windows) commit 926f422 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 01:45:51 2017 +0900 Fix checking args (for working on windows) commit 276a9cd Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 01:36:56 2017 +0900 syscall -> exec (for working on windows)

Squashed commit of the following: commit b677dbc Author: 3846masa <3846masahiro+git@gmail.com> Date: Sat Oct 28 11:35:30 2017 +0900 Fix golang version commit 43bc363 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sat Oct 28 11:12:42 2017 +0900 Fix shebang for Windows commit ed225df Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:29:39 2017 +0900 Fix CI commit 94a625d Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:26:49 2017 +0900 Fix windows installer commit 0bea8fb Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Sep 8 10:26:20 2017 +0900 Fix README for Windows installer commit 237a4a7 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 19:11:19 2017 +0900 Fix build for Windows commit bc508d7 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:59:16 2017 +0900 Fix to set API version (moby/moby#32779) commit 442be2f Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:03:01 2017 +0900 Update README for Windows commit e83b2ed Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:01:01 2017 +0900 Add installer for Windows commit 776b931 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 18:00:38 2017 +0900 Build for Windows commit bc01aef Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:43:43 2017 +0900 Fix `run.go` for Windows commit 63b9490 Merge: a72237f 0fea7e2 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:43:01 2017 +0900 Merge tag '0.1.0' of https://github.com/bfirsh/whalebrew into support-windows commit a72237f Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:26:29 2017 +0900 Fix `ForceInstall` for windows commit de458d9 Merge: 03d00bf e7d0792 Author: 3846masa <3846masahiro+git@gmail.com> Date: Thu Sep 7 17:22:41 2017 +0900 Merge tag '0.0.5' into support-windows commit 03d00bf Author: 3846masa <3846masahiro+git@gmail.com> Date: Tue Feb 14 00:01:52 2017 +0900 Fix batch file's shebang commit 21bcf14 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 22:19:25 2017 +0900 Fix .travis.yml for forked project commit a0e486c Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:59:19 2017 +0900 Add appveyor.yml commit fe06f35 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:34:46 2017 +0900 MakePackagePath commit bc12aa0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 21:23:31 2017 +0900 Rewrite test commit 7453d37 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 20:28:33 2017 +0900 `path.Join` -> `filepath.Join` commit 4ded1a2 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:53:51 2017 +0900 LF -> CRLF in batch file commit 8078165 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:34:08 2017 +0900 Fix edit for Windows commit 8746496 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:15:55 2017 +0900 Fix run for windows commit bb652d4 Merge: 8a6d783 e7e6427 Author: 3846masa <3846masahiro+git@gmail.com> Date: Mon Feb 13 16:08:42 2017 +0900 Merge v0.0.4 Merge remote-tracking branch 'origin/master' into support-windows commit 8a6d783 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 21:11:59 2017 +0900 Support to run and return status code on Windows commit ce3d298 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:58:21 2017 +0900 Revert "syscall -> exec (for working on windows)" This reverts commit 276a9cd. commit ffbdd15 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:50:08 2017 +0900 Add batch file support (for working on windows) commit cdc5a05 Author: 3846masa <3846masahiro+git@gmail.com> Date: Sun Feb 5 20:34:30 2017 +0900 Revert "Add batch file support (for working on windows)" This reverts commit aba79b0. commit 10e10a0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Fri Feb 3 00:53:23 2017 +0900 Revert "Fix checking args (for working on windows)" This reverts commit 926f422. commit 74db526 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 03:14:59 2017 +0900 Fix default WHALEBREW_INSTALL_PATH (for working on windows) commit aba79b0 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 03:06:00 2017 +0900 Add batch file support (for working on windows) commit 926f422 Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 01:45:51 2017 +0900 Fix checking args (for working on windows) commit 276a9cd Author: 3846masa <3846masahiro+git@gmail.com> Date: Wed Feb 1 01:36:56 2017 +0900 syscall -> exec (for working on windows)

GordonTheTurtle added the status/0-triage label Apr 22, 2017

AkihiroSuda added area/cli status/failing-ci Indicates that the PR in its current state fails the test suite labels Apr 25, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Apr 26, 2017

vdemeester requested review from dnephin and vieux April 26, 2017 13:20

dnephin reviewed Apr 26, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 28, 2017

dnephin reviewed Apr 28, 2017

View reviewed changes

rremer force-pushed the auto-update-client branch from 0cb5794 to e298205 Compare April 30, 2017 23:46

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 30, 2017

dnephin added rebuild/powerpc and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels May 1, 2017

GordonTheTurtle removed rebuild/powerpc labels May 1, 2017

dnephin added status/2-code-review and removed status/1-design-review labels May 1, 2017

dnephin approved these changes May 1, 2017

View reviewed changes

thaJeztah added the impact/cli label May 5, 2017

GordonTheTurtle assigned cpuguy83 May 7, 2017

rremer force-pushed the auto-update-client branch from e298205 to fe396be Compare June 13, 2017 01:21

dnephin reviewed Jun 13, 2017

View reviewed changes

moby deleted a comment from GordonTheTurtle Jun 13, 2017

thaJeztah mentioned this pull request Jun 15, 2017

Stop using deprecated SockRequest #33534

Merged

rremer force-pushed the auto-update-client branch 2 times, most recently from 174200d to 75b5de5 Compare June 16, 2017 00:02

dnephin approved these changes Jun 16, 2017

View reviewed changes

dnephin added the rebuild/windowsRS1 label Jun 16, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 16, 2017

dnephin approved these changes Jun 16, 2017

View reviewed changes

cpuguy83 requested changes Jun 20, 2017

View reviewed changes

pull client API version negotiation out of the CLI and into the client

5f1d94e

Signed-off-by: Royce Remer <royceremer@gmail.com>

rremer force-pushed the auto-update-client branch from 75b5de5 to 5f1d94e Compare June 21, 2017 05:58

cpuguy83 reviewed Jun 21, 2017

View reviewed changes

cpuguy83 approved these changes Jun 22, 2017

View reviewed changes

cpuguy83 merged commit 8a672ba into moby:master Jun 22, 2017

3846masa added a commit to 3846masa/whalebrew that referenced this pull request Sep 7, 2017

Fix to set API version (moby/moby#32779)

bc508d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pull client API version negotiation out of the CLI and into the client #32779

pull client API version negotiation out of the CLI and into the client #32779

rremer commented Apr 22, 2017

vdemeester commented Apr 26, 2017

dnephin left a comment

dnephin Apr 26, 2017

dnephin Apr 26, 2017

rremer Apr 28, 2017

dnephin Apr 28, 2017

dnephin Apr 26, 2017

dnephin Apr 26, 2017

dnephin Apr 26, 2017

rremer Apr 28, 2017

dnephin Apr 28, 2017

rremer commented Apr 28, 2017

dnephin Apr 28, 2017

rremer Apr 30, 2017

dnephin Apr 28, 2017

dnephin left a comment

cpuguy83 commented Jun 9, 2017

rremer commented Jun 13, 2017

dnephin Jun 13, 2017

rremer Jun 15, 2017

dnephin left a comment

rremer commented Jun 16, 2017

dnephin left a comment

rremer commented Jun 19, 2017

cpuguy83 Jun 20, 2017

cpuguy83 Jun 20, 2017

rremer Jun 21, 2017

cpuguy83 Jun 21, 2017

dnephin Jun 21, 2017

cpuguy83 left a comment

pull client API version negotiation out of the CLI and into the client #32779

pull client API version negotiation out of the CLI and into the client #32779

Conversation

rremer commented Apr 22, 2017

vdemeester commented Apr 26, 2017

dnephin left a 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

Choose a reason for hiding this comment

rremer commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

cpuguy83 commented Jun 9, 2017

rremer commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

rremer commented Jun 16, 2017

dnephin left a comment

Choose a reason for hiding this comment

rremer commented Jun 19, 2017

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 left a comment

Choose a reason for hiding this comment