WIP Handle multiple compose versions #854

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

cli/flags/common.go

+	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

cli/command/stack/kubernetes/check.go

+)
+
+// 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

cli/command/stack/kubernetes/check.go

+
+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

cli/command/stack/kubernetes/check.go

+	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

cli/command/stack/kubernetes/cli.go

+	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

cli/command/stack/kubernetes/cli.go

-	kubeConfig    *restclient.Config
-	kubeNamespace string
+	KubeConfig    *restclient.Config
+	KubeNamespace string
 }

 // 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

type Options struct { Namespace string Config string }

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

cli/command/system/version.go

@@ -75,12 +82,21 @@ type clientVersion struct {
 	Orchestrator      string `json:",omitempty"`
 }

+type kubernetesVersion struct {
+	Kubernetes string
+	ComposeAPI string


StackAPI not ComposeAPI

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

cli/command/system/version.go

+	}
+
+	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

cli/command/system/version.go

+		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

cli/command/system/version.go

@@ -109,7 +125,7 @@ func reformatDate(buildTime string) string {
 	return buildTime
 }

-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

cli/command/stack/kubernetes/cli.go

@@ -52,25 +51,27 @@ func WrapCli(dockerCli command.Cli, cmd *cobra.Command) (*KubeCli, error) {
 	if err != nil {
 		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

kubernetes/README.md

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


We should not remove that 👼

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

kubernetes/compose/v1beta1/scale.go

+	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

kubernetes/compose/v1beta1/stack_types.go

 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 <[email protected]>

* Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <[email protected]>

…use it kubernetes stack commands Signed-off-by: Silvin Lubecki <[email protected]>

Signed-off-by: Silvin Lubecki <[email protected]>

* Remove dependency from command in Kubernetes version checker Signed-off-by: Silvin Lubecki <[email protected]>

Signed-off-by: Silvin Lubecki <[email protected]>

* Refactor the internal stack model to use the v1beta2 stack model instead of compose type config Signed-off-by: Silvin Lubecki <[email protected]>

Signed-off-by: Silvin Lubecki <[email protected]>

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 versions Jan 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 60106c6 Compare February 1, 2018 13:55

dnephin reviewed Feb 1, 2018

View reviewed changes

vdemeester requested 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 68e09cb Compare 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 <[email protected]>

* Add kubernetes compose component v1beta2 client

c43492f

* Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <[email protected]>

Add a common interface between different compose client versions and …

3e257df

…use it kubernetes stack commands Signed-off-by: Silvin Lubecki <[email protected]>

Fix linter errors

b5d9ec2

Signed-off-by: Silvin Lubecki <[email protected]>

- Fix PR comments

7c7a3a3

* Remove dependency from command in Kubernetes version checker Signed-off-by: Silvin Lubecki <[email protected]>

Fix yaml marshaling

f768df9

Signed-off-by: Silvin Lubecki <[email protected]>

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 <[email protected]>

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

Fix linter errors and kubeconfig flag parsing

e653e9a

Signed-off-by: Silvin Lubecki <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Handle multiple compose versions #854

WIP Handle multiple compose versions #854

silvin-lubecki commented Jan 31, 2018

dnephin Jan 31, 2018

silvin-lubecki Feb 1, 2018 •

edited

Loading

dnephin Feb 1, 2018

codecov-io commented Feb 1, 2018 •

edited

Loading

dnephin left a comment

dnephin Feb 1, 2018 •

edited

Loading

silvin-lubecki Feb 2, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 2, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 19, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 19, 2018

dnephin Feb 1, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 19, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 20, 2018

dnephin Feb 1, 2018

silvin-lubecki Feb 20, 2018

dnephin Feb 1, 2018

dnephin Feb 1, 2018

silvin-lubecki commented Feb 2, 2018

vdemeester Feb 5, 2018

vdemeester Feb 5, 2018

vdemeester Feb 5, 2018

silvin-lubecki commented Feb 22, 2018

WIP Handle multiple compose versions #854

WIP Handle multiple compose versions #854

Conversation

silvin-lubecki commented Jan 31, 2018

Choose a reason for hiding this comment

silvin-lubecki Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 1, 2018 • edited Loading

Codecov Report

dnephin left a comment

Choose a reason for hiding this comment

dnephin Feb 1, 2018 • edited Loading

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

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 2, 2018

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 22, 2018

silvin-lubecki Feb 1, 2018 •

edited

Loading

codecov-io commented Feb 1, 2018 •

edited

Loading

dnephin Feb 1, 2018 •

edited

Loading