Bugfix: set cli.manualOverride when env var not empty

[WeiZhang555]

In function NewEnvClient, the cli.manualOverride will be set to true
always which is wrong, to fix it we should let manualOverride set to
true only if os.Getenv("DOCKER_API_VERSION") isn't empty.

One more step forward, even user set os.Getenv("DOCKER_API_VERSION"),
we should still allow updating client API version, in case that user set
a unreasonable version number.

If env var "DOCKER_API_VERSION" is specified by user, we'll set
cli.manualOverride, before this, this field is always true due to
wrong logic.

Signed-off-by: Zhang Wei zhangwei555@huawei.com

[WeiZhang555]

In function NewEnvClient, the cli.manualOverride will be set to true
always which is wrong,

You can see it in this piece of code:

 	version := os.Getenv("DOCKER_API_VERSION")
 	if version == "" {
 		version = DefaultVersion
 	}
 
 	cli, err := NewClient(host, version, client, nil)
  	if err != nil {
  		return cli, err
  	}
 	if version != "" {
 		cli.manualOverride = true
 	}

version is never an empty string, which will make cli.manualOverride = true

ping related person: @vieux

[vieux]

@WeiZhang555 it's is not set to true always, it's only set to true when the env var is not specified.

This is the intended behavior, is you specify a version in the env, we should not try to use anything else.

[WeiZhang555]

@vieux I mean in this piece of code:

 	version := os.Getenv("DOCKER_API_VERSION")
 	if version == "" {
 		version = DefaultVersion
 	}

if os.Getenv("DOCKER_API_VERSION") is empty, the version will be set to DefaultVersion (1.25), after that

    if version != "" {
 		cli.manualOverride = true
 	}

the if statement will always be true

[WeiZhang555]

This is the intended behavior, is you specify a version in the env, we should not try to use anything else.

I can accept it if this is exactly the design, what I'll suggest is modify this line:

https://github.com/docker/docker/blob/master/client/client.go#L125-L127

instead:

	if os.Getenv("DOCKER_API_VERSION") != "" {
		cli.manualOverride = true
	}

[WeiZhang555]

it's is not set to true always, it's only set to true when the env var is not specified.

Ooops, you mean env var is not specified? I thought it should be is specified. Are you making a typo or you really mean it ?

[vieux]

@WeiZhang555 you're right, it should be

if os.Getenv("DOCKER_API_VERSION") != "" {
		cli.manualOverride = true
	}

we set the manualOverride when DOCKER_API_VERSION is specified

[thaJeztah]

Hm, looks like I commented something like that, but it got lost in the last change there? #27745 (comment)

[WeiZhang555]

Let me make some update to fix it 😄

[thaJeztah]

My previous comment may not be related here (was discussing with @vieux) 😊 Thanks for addressing this @WeiZhang555

[WeiZhang555]

Updated, PTAL~~

[vieux]

LGTM thanks @WeiZhang555

[vdemeester]

LGTM 🐮