New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker version output is not consistent when there are downgrades or incompatibilities #30994 #31022

Merged
merged 1 commit into from Mar 31, 2017

Conversation

@jmzwcn
Contributor

jmzwcn commented Feb 15, 2017

[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

Show outdated Hide outdated api/types/types.go Outdated
Show outdated Hide outdated cli/command/system/version.go Outdated
@jmzwcn

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Feb 18, 2017

Contributor

@thaJeztah, @vieux, any new advice?

Contributor

jmzwcn commented Feb 18, 2017

@thaJeztah, @vieux, any new advice?

@vieux vieux self-assigned this Feb 19, 2017

@vieux vieux changed the title from [1.13] docker version output is not consistent when there are downgrades or incompatibilities #30994 to docker version output is not consistent when there are downgrades or incompatibilities #30994 Feb 21, 2017

@thaJeztah

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

@@ -76,14 +76,16 @@ func runVersion(dockerCli *command.DockerCli, opts *versionOptions) error {
}

This comment has been minimized.

@thaJeztah

thaJeztah Mar 27, 2017

Member

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

thaJeztah Mar 27, 2017

Member

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>).

This comment has been minimized.

@thaJeztah

thaJeztah Mar 27, 2017

Member

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

thaJeztah Mar 27, 2017

Member

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,
Show outdated Hide outdated api/types/client.go Outdated
Show outdated Hide outdated api/types/types.go Outdated
Show outdated Hide outdated cli/command/system/version.go Outdated
Docker version output is not consistent when there are downgrades or …
…incompatibilities.

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

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Mar 30, 2017

Contributor

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

Contributor

jmzwcn commented Mar 30, 2017

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

@thaJeztah

LGTM, thanks!

ping @vieux PTAL

@vdemeester

LGTM 🐯

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Mar 31, 2017

Collaborator

LGTM! thanks @jmzwcn

Collaborator

vieux commented Mar 31, 2017

LGTM! thanks @jmzwcn

@vieux vieux merged commit 3dfce9f into moby:master Mar 31, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32244 has succeeded
Details
janky Jenkins build Docker-PRs 40854 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1010 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11944 has succeeded
Details
z Jenkins build Docker-PRs-s390x 854 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 31, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#31022 from jmzwcn/issue30994
docker version output is not consistent when there are downgrades or incompatibilities moby#30994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment