--provider global flag for kompose #182

surajssd · 2016-10-03T11:20:28Z

Now a user can select a provider using global flag --provider=openshift to select openshift provider
or --provider-kubernetes to select kubernetes provider if nothing is provided kubernetes is the default provider.

Fixes #179

kadel

I think that it is not really user friendly to show options for all providers.

When you do kompose --provider kubernetes convert --h it should show only options that are valid for Kubernetes and when you do kompose --provider openshift convert --h user should see only options for OpenShift

kadel · 2016-10-03T20:36:24Z

Today on train to Berlin I've worked on same thing as part of work on kompose up for openshift. :-)

Because I wanted --help to show only options that are valid for given provider, I've done this slightly differently.

I've used app.Before to add Commands depending on specified provider.
You can see my wip in branch openshift-up in my fork master...kadel:openshift-up

I don't know how much time I'm going to have this week to clean it up and finish. But If you want to have provider flag sooner, look at my branch and you can use it ;-)

surajssd · 2016-10-04T05:16:51Z

@kadel thanks for pointers, this helps with the missing point I had!

surajssd · 2016-10-04T07:10:12Z

@kadel addressed the issues you mentioned!

Now output looks like this:

for openshift

$ kompose --provider=openshift convert -h
NAME:
   kompose convert - Convert Docker Compose file (e.g. docker-compose.yml) to Kubernetes objects

USAGE:
   kompose convert [command options] [arguments...]

OPTIONS:
   --out, -o                    Specify file name in order to save objects into [$OUTPUT_FILE]
   --replicas "1"               Specify the number of replicas in the generated resource spec (default 1)
   --chart, -c                  Create a chart deployment
   --yaml, -y                   Generate resource file in yaml format
   --stdout                     Print converted objects to stdout
   --deploymentconfig, --dc     Generate a DeploymentConfig for OpenShift

for k8s

$ kompose convert -h
NAME:
   kompose convert - Convert Docker Compose file (e.g. docker-compose.yml) to Kubernetes objects

USAGE:
   kompose convert [command options] [arguments...]

OPTIONS:
   --out, -o                            Specify file name in order to save objects into [$OUTPUT_FILE]
   --replicas "1"                       Specify the number of replicas in the generated resource spec (default 1)
   --chart, -c                          Create a chart deployment
   --yaml, -y                           Generate resource file in yaml format
   --stdout                             Print converted objects to stdout
   --deployment, -d                     Generate a deployment resource file (default on)
   --daemonset, --ds                    Generate a daemonset resource file
   --replicationcontroller, --rc        Generate a replication controller resource file

kadel

Thank you @surajssd.
Just couple suggestions ;-)

kadel · 2016-10-04T17:44:49Z

cli/app/app.go

+		if !opt.CreateD && !opt.CreateDS && !opt.CreateRC {
+			opt.CreateD = true
+		}
+		if opt.CreateDeploymentConfig {


this check is not needed, this can never happen

Oh yes, now this is not needed since command line handling will take care of it :)

kadel · 2016-10-04T17:45:56Z

cli/app/app.go

+		// create deploymentconfig by default if no controller has been set
+		if !opt.CreateDeploymentConfig {
+			opt.CreateDeploymentConfig = true
+		}
 		if opt.CreateD {


this check is not needed, this can never happen same for CreateDS and CreateRS

kadel · 2016-10-04T17:55:57Z

cli/command/command.go

+	provider := strings.ToLower(c.GlobalString("provider"))
+	switch provider {
+	case "kubernetes":
+		k8sControllers := []cli.Flag{


can we create whole new cli.Command here? So we can set usage help for every provider independently?
Or set commands.Usage here

If I run kompose --provider openshift convert -h It is still displaying usage mentioning Kubernetes

NAME: kompose convert - Convert Docker Compose file (e.g. docker-compose.yml) to Kubernetes objects ....

This is also relying on ConvertCommand always being first in that slice :-( This might lead to errors in the future. At least there should be comment in app.Commands

Yes sure we can create new cli.Command, and nice catch of that usage string BTW!

kadel · 2016-10-04T17:59:59Z

cli/main/main.go

@@ -33,7 +32,7 @@ func main() {
 	app.Author = "Skippbox Kompose Contributors"
 	app.Email = "https://github.com/skippbox/kompose"
 	app.EnableBashCompletion = true
-	app.Before = cliApp.BeforeApp
+	app.Before = command.BeforeApp
 	app.Flags = append(command.CommonFlags())
 	app.Commands = []cli.Command{
 		command.ConvertCommand(),


in command.BeforeApp you are counting on ConvertCommand being always in the first place here.
If you want it like that, here should be comment explaining that this cannot be moved.

I stuck to the way you did it, add the command itself in BeforeApp function, so this is not needed I think.

surajssd · 2016-10-05T04:35:18Z

Changed it now to have new cli.Command altogether, so usage is changed, added new function that returns common flags.

$ kompose --provider=openshift convert -h
NAME:
    - Convert Docker Compose file (e.g. docker-compose.yml) to OpenShift objects

USAGE:
    [command options] [arguments...]

OPTIONS:
   --deploymentconfig, --dc     Generate a OpenShift DeploymentConfig object
   --out, -o                    Specify file name in order to save objects into [$OUTPUT_FILE]
   --replicas "1"               Specify the number of replicas in the generated resource spec (default 1)
   --yaml, -y                   Generate resource file in yaml format
   --stdout                     Print converted objects to stdout

kadel

LGTM. Great work @surajssd 🎉

It would be great if @sebgoa or @ngtuna also look at this before it gets merged.

kadel · 2016-10-05T09:22:18Z

cli/command/command.go

+	return command
+}
+
+func commonConvertFlags() []cli.Flag {


surajssd · 2016-10-05T10:11:48Z

@kadel sure will wait for @sebgoa or @ngtuna to try it out once! Thanks for reviews and pointers!

ngtuna · 2016-10-05T10:18:33Z

Great work @surajssd 👍 I will try it today. Thanks

ngtuna · 2016-10-05T19:49:40Z

Hey @surajssd, at first test I see $ kompose --help lists out up and down command only. Convert is missing.

$ kompose --help
...
COMMANDS:
    up      Deploy your Dockerized application to Kubernetes (default: creating Kubernetes deployment and service)
    down    Delete instantiated services/deployments from kubernetes
...

ngtuna · 2016-10-05T20:04:49Z

Otherwise LGTM. I like the idea of changing usage between providers 💯

ngtuna · 2016-10-05T20:06:00Z

Then provider owners have their own space to arrange outputs.

Now a user can select a provider using global flag --provider=openshift to select openshift provider or --provider-kubernetes to select kubernetes provider if nothing is provided kubernetes is the default provider. Fixes kubernetes#179

surajssd · 2016-10-06T04:11:10Z

@ngtuna nice catch about convert not being visible. Made changes accordingly here https://github.com/skippbox/kompose/pull/182/files#diff-3145698b159fb0a026984b4a017a8b24R67 also added note here https://github.com/skippbox/kompose/pull/182/files#diff-d9814220427d68b7f7ade416b1440032R38

surajssd · 2016-10-06T04:13:17Z

And also it shows up under help

$ kompose -h
NAME:
[SNIP]
   Skippbox Kompose Contributors <https://github.com/skippbox/kompose> 

COMMANDS:
    convert     Convert Docker Compose file (e.g. docker-compose.yml) to Kubernetes/OpenShift objects
    up          Deploy your Dockerized application to Kubernetes (default: creating Kubernetes deployment and service)
    down        Delete instantiated services/deployments from kubernetes

GLOBAL OPTIONS:
[SNIP]

ngtuna · 2016-10-06T19:33:13Z

Thanks @surajssd . Merging now for the release tonight.

surajssd · 2016-10-07T04:25:24Z

Thanks @ngtuna !

surajssd added the review needed label Oct 3, 2016

kadel requested changes Oct 3, 2016

View reviewed changes

surajssd force-pushed the provider-flag branch from 5e31f05 to 5b74d54 Compare October 4, 2016 07:07

kadel requested changes Oct 4, 2016

View reviewed changes

surajssd force-pushed the provider-flag branch from 5b74d54 to 89385cd Compare October 5, 2016 04:28

kadel approved these changes Oct 5, 2016

View reviewed changes

cli/command/command.go

return command

}

func commonConvertFlags() []cli.Flag {

Copy link

Member

kadel Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

--provider global flag for kompose

b969f7a

Now a user can select a provider using global flag --provider=openshift to select openshift provider or --provider-kubernetes to select kubernetes provider if nothing is provided kubernetes is the default provider. Fixes kubernetes#179

surajssd force-pushed the provider-flag branch from 89385cd to b969f7a Compare October 6, 2016 04:09

ngtuna merged commit fbfdb75 into kubernetes:master Oct 6, 2016

surajssd mentioned this pull request Dec 1, 2016

Switch to spf13/cobra from urfave/cli #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--provider global flag for kompose #182

--provider global flag for kompose #182

surajssd commented Oct 3, 2016

kadel left a comment

kadel commented Oct 3, 2016

surajssd commented Oct 4, 2016

surajssd commented Oct 4, 2016

kadel left a comment

kadel Oct 4, 2016

surajssd Oct 5, 2016

kadel Oct 4, 2016

kadel Oct 4, 2016 •

edited

Loading

surajssd Oct 5, 2016

kadel Oct 4, 2016

surajssd Oct 5, 2016

surajssd commented Oct 5, 2016

kadel left a comment

kadel Oct 5, 2016

surajssd commented Oct 5, 2016 •

edited

Loading

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

surajssd commented Oct 6, 2016

surajssd commented Oct 6, 2016

ngtuna commented Oct 6, 2016

surajssd commented Oct 7, 2016

--provider global flag for kompose #182

--provider global flag for kompose #182

Conversation

surajssd commented Oct 3, 2016

kadel left a comment

Choose a reason for hiding this comment

kadel commented Oct 3, 2016

surajssd commented Oct 4, 2016

surajssd commented Oct 4, 2016

kadel left a comment

Choose a reason for hiding this comment

kadel Oct 4, 2016

Choose a reason for hiding this comment

surajssd Oct 5, 2016

Choose a reason for hiding this comment

kadel Oct 4, 2016

Choose a reason for hiding this comment

kadel Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

surajssd Oct 5, 2016

Choose a reason for hiding this comment

kadel Oct 4, 2016

Choose a reason for hiding this comment

surajssd Oct 5, 2016

Choose a reason for hiding this comment

surajssd commented Oct 5, 2016

kadel left a comment

Choose a reason for hiding this comment

kadel Oct 5, 2016

Choose a reason for hiding this comment

surajssd commented Oct 5, 2016 • edited Loading

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

ngtuna commented Oct 5, 2016

surajssd commented Oct 6, 2016

surajssd commented Oct 6, 2016

ngtuna commented Oct 6, 2016

surajssd commented Oct 7, 2016

kadel Oct 4, 2016 •

edited

Loading

surajssd commented Oct 5, 2016 •

edited

Loading