[jmzwcn]

[1.13] docker version output is not consistent when there are downgrades or incompatibilities #30994

Signed-off-by: Daniel Zhang jmzwcn@gmail.com

[thaJeztah]

left some comments

also ping @vieux

[thaJeztah]

I think we should keep naming this APIVersion as it's also used in --format

[thaJeztah]

In that case APIVersion does not need the omitempty as well

[thaJeztah]

We should keep the message the same here ("downgraded from").

Can we make make the actual comparison in this template? i.e.; if actual != default, then print the "downgraded from .." part? I'm not too good in Go templates, but I think it should be possible

[jmzwcn]

What is the difference with old way? Or I don't really understand "docker version output is not consistent".

[febbraro]

When I wrote not consistent I meant that when there is no downgrade it outputs "1.26" but when it downgrades it prints out "1.24 (downgraded from 1.26)" and when you are using --format to grab actual values in code that makes it challenging (and inconsistent) when .Client.APIVersion can be in a variety of formats.

[jmzwcn]

@thaJeztah, @vieux, any new advice?

[thaJeztah]

thanks @jmzwcn - I left my comments inline; let me know if you have questions, or suggestions (perhaps I overlooked something 😊)

[thaJeztah]

My thought was to make the (downgraded from ...) part of the template, that way, "presentation" is separate from the "data". Doing so, allows the --format tag to use the "raw" version, without having a rather "meaningless" <version> (downgraded from <other version>).

[thaJeztah]

Created a quick change to show my current thoughts;

• move the API types from api/types.. to the CLI (as they're not used in the actual API)
• moved the formatting ((downgraded from..)) to the template

With this, the default output on the CLI still looks the same, but --format allows you to use the actual "versions", or allows you to output a decent JSON;

docker version --format '{{ json . }}' | jq .
{
  "Client": {
    "Version": "1.14.0-dev",
    "ApiVersion": "1.12",
    "DefaultAPIVersion": "1.27",
    "GitCommit": "c79475f4e",
    "GoVersion": "go1.7.5",
    "Os": "linux",
    "Arch": "amd64",
    "BuildTime": "Mon Mar 27 21:27:09 2017"
  },
  "Server": {
    "Version": "1.14.0-dev",
    "ApiVersion": "1.27",
    "MinAPIVersion": "1.12",
    "GitCommit": "c79475f4e",
    "GoVersion": "go1.7.5",
    "Os": "linux",
    "Arch": "amd64",
    "KernelVersion": "4.9.12-moby",
    "BuildTime": "Mon Mar 27 21:27:09 2017"
  }
}

Diff from your current PR looks like this;

diff --git a/api/types/client.go b/api/types/client.go
index 8fa731614..698c56599 100644
--- a/api/types/client.go
+++ b/api/types/client.go
@@ -256,18 +256,6 @@ type ResizeOptions struct {
 	Width  uint
 }
 
-// VersionResponse holds version information for the client and the server
-type VersionResponse struct {
-	Client *ClientVersion
-	Server *Version
-}
-
-// ServerOK returns true when the client could connect to the docker server
-// and parse the information received. It returns false otherwise.
-func (v VersionResponse) ServerOK() bool {
-	return v.Server != nil
-}
-
 // NodeListOptions holds parameters to list nodes with.
 type NodeListOptions struct {
 	Filters filters.Args
diff --git a/api/types/types.go b/api/types/types.go
index 4d90f746c..520eb77b9 100644
--- a/api/types/types.go
+++ b/api/types/types.go
@@ -115,21 +115,6 @@ type Version struct {
 	BuildTime     string `json:",omitempty"`
 }
 
-// ClientVersion contains response of Client API:
-type ClientVersion struct {
-	Version           string
-	APIVersion        string `json:"ApiVersion"`
-	DefaultAPIVersion string `json:"DefaultAPIVersion,omitempty"`
-	ActualAPIVersion  string `json:"ActualAPIVersion,omitempty"`
-	GitCommit         string
-	GoVersion         string
-	Os                string
-	Arch              string
-	KernelVersion     string `json:",omitempty"`
-	Experimental      bool   `json:",omitempty"`
-	BuildTime         string `json:",omitempty"`
-}
-
 // Commit holds the Git-commit (SHA1) that a binary was built from, as reported
 // in the version-string of external tools, such as containerd, or runC.
 type Commit struct {
diff --git a/cli/command/system/version.go b/cli/command/system/version.go
index 99426bd19..468db7d03 100644
--- a/cli/command/system/version.go
+++ b/cli/command/system/version.go
@@ -1,7 +1,6 @@
 package system
 
 import (
-	"fmt"
 	"runtime"
 	"time"
 
@@ -17,7 +16,7 @@ import (
 
 var versionTemplate = `Client:
  Version:      {{.Client.Version}}
- API version:  {{if eq .Client.DefaultAPIVersion .Client.ActualAPIVersion}}{{.Client.DefaultAPIVersion}}{{else}}{{.Client.APIVersion}}{{end}}
+ API version:  {{.Client.APIVersion}}{{if ne .Client.APIVersion .Client.DefaultAPIVersion}} (downgraded from {{.Client.DefaultAPIVersion}}){{end}}
  Go version:   {{.Client.GoVersion}}
  Git commit:   {{.Client.GitCommit}}
  Built:        {{.Client.BuildTime}}
@@ -36,6 +35,29 @@ type versionOptions struct {
 	format string
 }
 
+// versionInfo contains version information of both the Client, and Server
+type versionInfo struct {
+	Client clientVersion
+	Server *types.Version
+}
+
+type clientVersion struct {
+	Version           string
+	APIVersion        string `json:"ApiVersion"`
+	DefaultAPIVersion string `json:"DefaultAPIVersion,omitempty"`
+	GitCommit         string
+	GoVersion         string
+	Os                string
+	Arch              string
+	BuildTime         string `json:",omitempty"`
+}
+
+// ServerOK returns true when the client could connect to the docker server
+// and parse the information received. It returns false otherwise.
+func (v versionInfo) ServerOK() bool {
+	return v.Server != nil
+}
+
 // NewVersionCommand creates a new cobra.Command for `docker version`
 func NewVersionCommand(dockerCli *command.DockerCli) *cobra.Command {
 	var opts versionOptions
@@ -70,17 +92,11 @@ func runVersion(dockerCli *command.DockerCli, opts *versionOptions) error {
 			Status: "Template parsing error: " + err.Error()}
 	}
 
-	APIVersion := dockerCli.Client().ClientVersion()
-	if defaultAPIVersion := dockerCli.DefaultVersion(); APIVersion != defaultAPIVersion {
-		APIVersion = fmt.Sprintf("%s (downgraded from %s)", APIVersion, defaultAPIVersion)
-	}
-
-	vd := types.VersionResponse{
-		Client: &types.ClientVersion{
+	vd := versionInfo{
+		Client: clientVersion{
 			Version:           dockerversion.Version,
-			APIVersion:        APIVersion,
+			APIVersion:        dockerCli.Client().ClientVersion(),
 			DefaultAPIVersion: dockerCli.DefaultVersion(),
-			ActualAPIVersion:  dockerCli.Client().ClientVersion(),
 			GoVersion:         runtime.Version(),
 			GitCommit:         dockerversion.GitCommit,
 			BuildTime:         dockerversion.BuildTime,

[thaJeztah]

I realize this was already there, but looking at the VersionResponse type; I don't think it belongs in api/types/.., because it's not used in the actual API, only in the CLI

[thaJeztah]

Same for this ClientVersion type; it's only used in the CLI (not in the API / API-client); I think we should move this.

[jmzwcn]

Great, I will follow it and take it soon.

[thaJeztah]

We should make it so that the default CLI output shows the format as we have now, but allow --format to access the individual parts (also see my other comment)

[jmzwcn]

@thaJeztah, already done, please let me know if there is any problem

[thaJeztah]

LGTM, thanks!

ping @vieux PTAL

[vdemeester]

LGTM 🐯

[vieux]

LGTM! thanks @jmzwcn