WIP Handle multiple compose versions by silvin-lubecki · Pull Request #854 · docker/cli

silvin-lubecki · 2018-01-31T17:18:14Z

- What I did
A new compose component for Kubernetes version has been released (v1beta2), and I added support for both versions. I also added an Orchestrator section in the docker version command to log the compose component version and Kubernetes version we are working against.

$ docker version
Client:
Version: 18.02.0-dev
API version: 1.35 (downgraded from 1.36)
Go version: go1.9.3
Git commit: 0d6a70514
Built: Wed Jan 31 16:22:38 2018
OS/Arch: darwin/amd64
Experimental: true
Orchestrator: kubernetes
Server:
Engine:
Version: 18.01.0-ce
API version: 1.35 (minimum version 1.12)
Go version: go1.9.2
Git commit: 03596f5
Built: Wed Jan 10 20:13:12 2018
OS/Arch: linux/amd64
Experimental: true
Orchestrator:
Kubernetes: v1.8.2
Compose: v1beta2

- How I did it
I put an interface on the compose client used by docker stack subcommands and added an implementation for each compose version.

- How to verify it

- Description for the changelog

New Orchestrator section in docker version command and added support of kubernetes compose component v1beta2

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

dnephin · 2018-01-31T23:20:02Z

+	flags.SetAnnotation("namespace", "experimentalCLI", nil)
+	flags.String("kubeconfig", "", "Kubernetes config file")
+	flags.SetAnnotation("kubeconfig", "kubernetes", nil)
+	flags.SetAnnotation("kubeconfig", "experimentalCLI", nil)


Why do these need to move to common? They aren't common to both orchestrators

Well I mostly need kubeconfig (not namespace) to be common, or at least available, for docker version command, as you may specify different kubernetes configs and check the version against the pointed kubernetes.

I see, but this flag isn't added to the version command, so how is it going to be read?

codecov-io · 2018-02-01T13:48:55Z

Codecov Report

Merging #854 into master will decrease coverage by 0.45%.
The diff coverage is 23.31%.

@@ Coverage Diff @@## master #854 +/- ##
=========================================
- Coverage 53.26% 52.8% -0.46% 
=========================================
Files 258 263 +5 Lines 16357 16702 +345 =========================================
+ Hits 8712 8820 +108 - Misses 7081 7316 +235 - Partials 564 566 +2

dnephin

This is a large PR, and it seems there are two logical changes here. 1) add kube version/stack api version to the API 2) update to include a new API version.

Could you split this into two separate PRs so it's easier to review?

dnephin · 2018-02-01T21:49:25Z

+)
+
+// GetAPIVersion returns the most recent stack API installed.
+func (c *KubeCli) GetAPIVersion() (StackVersion, error) {


I don't think this should be a method on KubeCLI. It only uses KubeConfig, so it could be just a function which accepts a config and returns the version.
This should make it much easier to use from the version command.
I think it will also remove the need to export KubeConfig

Right, I'll change this.

dnephin · 2018-02-01T21:50:03Z

+
+const (
+	// StackNotFound is returned when no stack api at all is detected on kubernetes.
+	StackNotFound = "notFound"


Shouldn't this be an error instead of a string? It looks like it's only used in the error case, so this const probably doesn't need to exist. Just use "".

Ok! It was only to be consistent with the "enum" values. But "" is fine.

dnephin · 2018-02-01T21:51:48Z

+	case findVersion(apiv1beta1.SchemeGroupVersion, groups.Groups):
+ return StackAPIV1Beta1, nil
+	default:
+ return StackNotFound, fmt.Errorf("could not find %s api. Install it on your cluster first", apiv1beta1.SchemeGroupVersion.Group)


s/fmt.Errorf/errors.Errorf/ (from pkg/errors)
This prints the V1beta1 api version, which might be confusing if the user is expecting to use v2. Maybe it should just say "failed to find a Stack API version" or something like that?

dnephin · 2018-02-01T21:54:11Z

+	case StackAPIV1Beta2:
+ return c.newStackV1Beta2()
+	default:
+ return nil, fmt.Errorf("could not find matching Stack API version")


s/fmt.Errorf/errors.New/
Maybe something like "No supported Stack API version" ?

dnephin · 2018-02-01T21:58:16Z

 }

 // WrapCli wraps command.Cli with kubernetes specifics
 func WrapCli(dockerCli command.Cli, cmd *cobra.Command) (*KubeCli, error) {


Instead of accepting a *cobra.Command I think this should accept a
typeOptionsstruct { NamespacestringConfigstring }
the Options struct would be populated by flag parsing before calling WrapCli().
cobra.Command is a huge struct and we don't use anything but a single method (.Flags()), depending on the whole struct makes it harder to re-use (as we can see from the version command).
It should also make the code more obvious. Right now it's assumed that this will get the flag value, but in reality, when used with a different cmd the flag is never presented to the user, so the flag will never be set.

dnephin · 2018-02-01T22:11:58Z


+type kubernetesVersion struct {
+	Kubernetes string
+	ComposeAPI string


StackAPI not ComposeAPI

dnephin · 2018-02-01T22:12:37Z

+	}
+
+	apiVersion, err := kli.GetAPIVersion()
+	if err != nil {


If this fails it should still attempt to get the kube version, so maybe this should be split into two functions.

Right, done

dnephin · 2018-02-01T22:13:56Z

+ return nil, nil
+	}
+
+	kli, err := kubernetes.WrapCli(dockerCli, cmd)


Instead of WrapCLI() if this could call a method that returns KubeConfig then it can be used for both GetAPIVersion and getting a kubeClient without the need for KubeCli.

dnephin · 2018-02-01T22:15:11Z

 }

-func runVersion(dockerCli command.Cli, opts *versionOptions) error {
+func runVersion(dockerCli command.Cli, cmd *cobra.Command, opts *versionOptions) error {


I think we should not be passing in cobra.Command to the run functions. With the other suggestions (refactoring to remove the need to use KubeCli to get version, and getting the kube config from a path string arg instead of flags this shouldn't be necessary any more and can be reverted.

dnephin · 2018-02-01T22:17:44Z

 return nil, fmt.Errorf("Failed to load kubernetes configuration file '%s'", kubeConfig)
 	}
-	cli.kubeConfig = config
+	cli.KubeConfig = config


Related to other changes, this "get KubeConfig from flag or env" can be extracted into a function that just takes a single path string argument.

silvin-lubecki · 2018-02-02T17:55:22Z

@dnephin Let me just fix your comments before I split it into 2 PR? Otherwise they may "disappear".

vdemeester · 2018-02-05T06:49:47Z

@@ -1,4 +0,0 @@
-# Kubernetes client libraries


We should not remove that 👼

vdemeester · 2018-02-05T06:52:16Z

+	yaml "gopkg.in/yaml.v2"
+)
+
+// Scale scale a service to the specified number of replicas


v1beta1 didn't support scale, it should still not support it (i.e. it comes in v1beta2).. so this should not be required

vdemeester · 2018-02-05T06:53:17Z

-// +resource:path=stacks,strategy=StackStrategy
-// +subresource:request=Owner,path=owner,rest=OwnerStackREST
 type Stack struct {
+	StackImpl


I don't think this is required at all 🤔 (there shouldn't be any code change in v1beta1)

…nent version in docker version command. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

* Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

…use it kubernetes stack commands Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

* Remove dependency from command in Kubernetes version checker Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

* Refactor the internal stack model to use the v1beta2 stack model instead of compose type config Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki · 2018-02-22T14:59:54Z

Split and reopened in #898 and #899

silvin-lubecki requested review from dnephin and vdemeester as code owners January 31, 2018 17:18

GordonTheTurtle added the status/0-triage label Jan 31, 2018

silvin-lubecki changed the title ~~Handle multiple compose versions~~WIP Handle multiple compose versionsJan 31, 2018

vdemeester added status/1-design-review and removed status/0-triage labels Jan 31, 2018

vdemeester requested a review from thaJeztah January 31, 2018 17:19

dnephin reviewed Jan 31, 2018
View reviewed changes

silvin-lubecki force-pushed the handle-multiple-compose-versions branch 2 times, most recently from 933a508 to 60106c6Compare February 1, 2018 13:55

dnephin reviewed Feb 1, 2018
View reviewed changes

vdemeester suggested changes Feb 5, 2018
View reviewed changes

vdemeester added area/stack area/kubernetes labels Feb 5, 2018

silvin-lubecki force-pushed the handle-multiple-compose-versions branch 4 times, most recently from f14ed2f to 68e09cbCompare February 21, 2018 16:45

silvin-lubecki added 7 commits February 22, 2018 11:01

Add Orchestrator subsection with kubernetes version and compose compo…
4e12130
…nent version in docker version command. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

* Add kubernetes compose component v1beta2 client
c43492f
* Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Add a common interface between different compose client versions and …
3e257df
…use it kubernetes stack commands Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Fix linter errors
b5d9ec2
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

- Fix PR comments
7c7a3a3
* Remove dependency from command in Kubernetes version checker Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Fix yaml marshaling
f768df9
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Bump kubernetes stack client v1beta2
2e8f25d
* Refactor the internal stack model to use the v1beta2 stack model instead of compose type config Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki force-pushed the handle-multiple-compose-versions branch from 68e09cb to 2e8f25dCompare February 22, 2018 10:37

Fix linter errors and kubeconfig flag parsing
e653e9a
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki mentioned this pull request Feb 22, 2018
Print Stack API and Kubernetes versions in version command #898
Merged

silvin-lubecki closed this Feb 22, 2018

silvin-lubecki deleted the handle-multiple-compose-versions branch May 30, 2018 14:29

Uh oh!

Conversation

silvin-lubecki commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubeckiFeb 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephinFeb 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

silvin-lubeckiFeb 1, 2018•
edited
Loading

codecov-io commented Feb 1, 2018•
edited
Loading

dnephinFeb 1, 2018•
edited
Loading