Print Stack API and Kubernetes versions in version command #898

silvin-lubecki · 2018-02-22T14:04:57Z

- What I did
This pull request adds the Stack API resolution mechanism and print the selected version into the version command. It lays the foundation for a following one #899, which adds the v1beta2 version of the Stack API. It's extracted from the PR #854 .

- How I did it

Resolve Stack API using Kubernetes discovering API
Refactor Kubernetes flags parsing

- How to verify it
Execute a docker version command against latest docker for mac, it should output at the end:

 Orchestrator:
   Kubernetes:  v1.9.2
   Stack:       v1beta1

- Description for the changelog

Add Stack API and Kubernetes versions in version command

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

codecov-io · 2018-02-22T14:10:50Z

Codecov Report

Merging #898 into master will decrease coverage by 0.05%.
The diff coverage is 23.91%.

@@            Coverage Diff            @@
##           master    #898      +/-   ##
=========================================
- Coverage   53.26%   53.2%   -0.06%     
=========================================
  Files         258     259       +1     
  Lines       16357   16415      +58     
=========================================
+ Hits         8712    8734      +22     
- Misses       7081    7117      +36     
  Partials      564     564

vdemeester · 2018-02-23T13:20:46Z

cli/command/system/version.go

+ {{- if .KubernetesOK}}{{with .Kubernetes}}
+ Orchestrator:
+   Kubernetes:	{{.Kubernetes}}
+   Stack:	{{.StackAPI}}{{- end}}{{end}}`


What if there is multiple API version available ? Should we display sthg a bit like docker api versions (version negociated vs min version) or a list of version, of just the version used ?

I don't see the benefits of listing all the versions? At least with just the version used, there is no ambiguity.

vdemeester · 2018-02-23T13:23:08Z

kubernetes/check.go

+)
+
+// GetAPIVersion returns the most recent stack API installed.
+func GetAPIVersion(kubeConfig *restclient.Config) (StackVersion, error) {


We may want to use GetStackAPIVersion here, as it's only the Stack api version we're getting here. We could have more api extension in the future, and kubernetes.GetAPIVersion kinda makes think it's the kubernetes api (server) version.

silvin-lubecki · 2018-02-23T14:52:13Z

@vdemeester PTAL

vdemeester

LGTM 🐯

dnephin

Thanks! This is looking good, just a couple small nits on the template format.

dnephin · 2018-02-23T18:36:18Z

kubernetes/check.go

+	clients, err := kubernetes.NewForConfig(kubeConfig)
+	if err != nil {
+		return "", err
+	}


This function could accept clients instead of a config. The client is being created twice.

Good catch, fixed.

dnephin · 2018-02-23T18:37:00Z

cli/command/system/version.go

+
+func getKubernetesServerVersion(kubeConfig *restclient.Config) string {
+	version := "Unknown"
+	kubeClient, err := kubernetesClient.NewForConfig(kubeConfig)


Could accept kubeClient instead of config, so the client can be passed to GetStackAPIVersion as well.

dnephin · 2018-02-23T18:39:23Z

cli/command/system/version.go

+ {{- end}}{{- end}}
+ {{- if .KubernetesOK}}{{with .Kubernetes}}
+ Orchestrator:
+   Kubernetes:	{{.Kubernetes}}


Minor naming nit: I assume in the future we will need to change "Server" to "Engine".

Maybe this should be formatted as:

Kubernetes: Version: {{.Kubernetes}} Stack API: {{.StackAPI}}

I think that would be more consistent with the other sections. What do you think?

My first thought was to generalize this section to all orchestrators, but finally it seems that we don't have much things to print from swarm, and also this section seems very specific to Kubernetes. So I'm with renaming it.

dnephin · 2018-02-23T18:40:20Z

cli/command/system/version.go

+ {{- if .KubernetesOK}}{{with .Kubernetes}}
+ Orchestrator:
+   Kubernetes:	{{.Kubernetes}}
+   Stack:	{{.StackAPI}}{{- end}}{{end}}`


Minor formatting nit: the {{- end}}{{end}} can go on a new line so that the template reads a bit better.

- in a template means "remove all whitespace before this token, so the trailing newline wont exist in the final rendered string.

Thanks, I wasn't aware of that, fixed.

dnephin · 2018-02-23T18:43:10Z

cli/command/stack/deploy.go

@@ -20,7 +20,7 @@ func newDeployCommand(dockerCli command.Cli) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.Namespace = args[0]
 			if dockerCli.ClientInfo().HasKubernetes() {
-				kli, err := kubernetes.WrapCli(dockerCli, cmd)
+				kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(cmd.Flags()))


I hope that in the future when we add kube to more commands we can have this accept an Options struct instead of cmd.Flags().

Ok I'll refactor that in a future PR.

* Resolve Stack API using Kubernetes discovering API * Refactor Kubernetes flags parsing Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki · 2018-02-26T00:17:37Z

PTAL @dnephin

dnephin

LGTM

thaJeztah

LGTM

silvin-lubecki requested review from dnephin and vdemeester as code owners February 22, 2018 14:04

GordonTheTurtle added the status/0-triage label Feb 22, 2018

vdemeester requested a review from thaJeztah February 22, 2018 14:06

vdemeester added status/2-code-review and removed status/0-triage labels Feb 22, 2018

This was referenced Feb 22, 2018

Handle multiple stack api versions #899

Merged

WIP Handle multiple compose versions #854

Closed

vdemeester requested changes Feb 23, 2018

View reviewed changes

silvin-lubecki force-pushed the add-stack-to-version-command branch from 4a4815a to a4e7e6c Compare February 23, 2018 14:43

vdemeester approved these changes Feb 23, 2018

View reviewed changes

dnephin reviewed Feb 23, 2018

View reviewed changes

Print Stack API version in version command

854aad8

* Resolve Stack API using Kubernetes discovering API * Refactor Kubernetes flags parsing Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki force-pushed the add-stack-to-version-command branch from a4e7e6c to 854aad8 Compare February 26, 2018 00:17

dnephin approved these changes Feb 26, 2018

View reviewed changes

vdemeester added the area/kubernetes label Feb 27, 2018

thaJeztah approved these changes Feb 27, 2018

View reviewed changes

vdemeester merged commit 2851c00 into docker:master Feb 27, 2018

GordonTheTurtle added this to the 18.04.0 milestone Feb 27, 2018

silvin-lubecki deleted the add-stack-to-version-command branch February 28, 2018 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print Stack API and Kubernetes versions in version command #898

Print Stack API and Kubernetes versions in version command #898

silvin-lubecki commented Feb 22, 2018 •

edited

Loading

codecov-io commented Feb 22, 2018 •

edited

Loading

vdemeester Feb 23, 2018

silvin-lubecki Feb 23, 2018

vdemeester Feb 23, 2018

silvin-lubecki Feb 23, 2018

silvin-lubecki commented Feb 23, 2018

vdemeester left a comment

dnephin left a comment

dnephin Feb 23, 2018

silvin-lubecki Feb 25, 2018

dnephin Feb 23, 2018

silvin-lubecki Feb 25, 2018

dnephin Feb 23, 2018

silvin-lubecki Feb 25, 2018

dnephin Feb 23, 2018

silvin-lubecki Feb 25, 2018

dnephin Feb 23, 2018

silvin-lubecki Feb 26, 2018

silvin-lubecki commented Feb 26, 2018

dnephin left a comment

thaJeztah left a comment

Print Stack API and Kubernetes versions in version command #898

Print Stack API and Kubernetes versions in version command #898

Conversation

silvin-lubecki commented Feb 22, 2018 • edited Loading

codecov-io commented Feb 22, 2018 • edited Loading

Codecov Report

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

silvin-lubecki commented Feb 23, 2018

vdemeester left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

silvin-lubecki commented Feb 26, 2018

dnephin left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Feb 22, 2018 •

edited

Loading

codecov-io commented Feb 22, 2018 •

edited

Loading