Switch to spf13/cobra from urfave/cli #304

cdrage · 2016-11-21T19:40:53Z

There's A LOT happening in this commit, so here's an outline:

First off, urfave/cli has been removed in favour of spf13/cobra. With
this, comes changes to the formatting as well as the help page for
Kompose.
Upon converting, I noticed a CLI flag was NOT appearing for OpenShift.
Specifically, --deploymentconfig. This has been added with a note
that says it is OpenShift only.
Exit codes have been fixed. If the conversion / down / up fails for
any reason, Kompose will exit with Code 1.
--verbose as well as --suppress-warnings can now be set at the
same time.
app_test.go in the cli directory has been moved to pkg/transformer
to better reflect the testing coverage.
version.go has been removed and converted to it's own CLI command in
conjuction with (most) Go software. A new CLI command has been
created. kompose version
--dab isn't a conventional way for short-form CLI paramters. This
has been shortened to -b for bundle.
CLI flags consisting of only two/three letters have been removed due to
it being unconventional for CLI. For example, --dc was removed in preference
for --deploymentconfig
--replicas has been added as an option when using kompose down or
kompose up. This has been added as previously in app.go the
replica amount was hard-coded as 1.
Differentiating names have been used for flags. For example,
persistent flags use the name Global (ex. GlobalOut). Command-specific
flags have their own names (ex. UpOpt).

Closes #239 #253

cdrage · 2016-11-21T19:41:31Z

@kadel I'll need some help in regards to vendoring in spf13/cobra if you can help me with the commands :)

cdrage · 2016-11-22T13:57:33Z

@kadel @ngtuna
Would need some help regarding the failing bash / cmd tests:

I don't touch anything regarding the "conversion". Despite having the exact same output at before the conversion:

▶ ./kompose --provider=openshift -f $GOPATH/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad/docker-compose.yml convert --stdout
WARN[0000] The DB_PORT variable is not set. Substituting a blank string. 
WARN[0000] The DB_USER variable is not set. Substituting a blank string. 
WARN[0000] The DB_HOST variable is not set. Substituting a blank string. 
WARN[0000] The DB_NAME variable is not set. Substituting a blank string. 
WARN[0000] The DB_PASS variable is not set. Substituting a blank string. 
WARN[0000] The DB_PORT variable is not set. Substituting a blank string. 
WARN[0000] The DB_NAME variable is not set. Substituting a blank string. 
WARN[0000] The DB_PASS variable is not set. Substituting a blank string. 
WARN[0000] The DB_USER variable is not set. Substituting a blank string. 
WARN[0000] The ROOT_PASS variable is not set. Substituting a blank string. 
ERRO[0000] Could not parse config for project etherpad : Service 'mariadb' configuration key 0 value Does not match format 'ports' 
FATA[0000] Failed to load compose file: Service 'mariadb' configuration key 0 value Does not match format 'ports'

It still fails with the

convert::expect_success_and_warning "kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/etherpad/docker-compose.yml convert --stdout" "$KOMPOSE_ROOT/script/test/fixtures/etherpad/output-os.json" "Unsupported key depends_on - ignoring"

test.

Did something change?

surajssd · 2016-11-23T16:56:03Z

@cdrage export envs before you run kompose

✔ ~/git/gowork/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad [master ↑·307|…1] 
09:49 $ export $(cat envs )    

✔ ~/git/gowork/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad [master ↑·307|…1] 
09:51 $ kompose --provider openshift up
WARN[0000] Unsupported key depends_on - ignoring        
We are going to create OpenShift DeploymentConfigs, Services and PersistentVolumeClaims for your Dockerized application. 
If you need different kind of resources, use the 'kompose convert' and 'oc create -f' commands instead. 

INFO[0000] Successfully created Service: etherpad       
INFO[0000] Successfully created Service: mariadb        
INFO[0000] Successfully created DeploymentConfig: etherpad 
INFO[0000] Successfully created ImageStream: etherpad   
INFO[0000] Successfully created DeploymentConfig: mariadb 
INFO[0000] Successfully created ImageStream: mariadb    
INFO[0000] Successfully created PersistentVolumeClaim: mariadb-claim0 

Your application has been deployed to OpenShift. You can run 'oc get dc,svc,is,pvc' for details.

cdrage · 2016-11-23T16:58:41Z

@surajssd not quite sure what you mean when exporting envs.

Isn't this already in the testing suite / shouldn't fail?

edit: it also looks like you're testing against master not PR #304
edit2: also, I'm testing the failures of convert in the testing, not up

surajssd · 2016-11-24T13:53:30Z

@cdrage this is what i did and found out it works with openshift

✔ ~/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad [switch-to-cobra|✔] 
19:14 $ ll
total 36
-rw-rw-r--. 1 hummer hummer  102 Oct 24 16:11 docker-compose-no-image.yml
-rw-rw-r--. 1 hummer hummer   61 Oct 24 16:11 docker-compose-no-ports.yml
-rw-rw-r--. 1 hummer hummer  509 Nov 24 19:06 docker-compose.yml
-rw-rw-r--. 1 hummer hummer   99 Oct 24 16:11 envs
-rw-rw-r--. 1 hummer hummer 4852 Nov 24 19:06 output-k8s.json
-rw-rw-r--. 1 hummer hummer 7119 Nov 24 19:06 output-os.json
-rw-rw-r--. 1 hummer hummer  180 Oct 24 16:11 README.md
✔ ~/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad [switch-to-cobra|✔] 
19:14 $ export $(cat envs)                                                                                                                                                                                        
✔ ~/go/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad [switch-to-cobra|✔] 
19:14 $ kompose --provider openshift convert --stdout                                                                                                                                                              
WARN[0000] Unsupported key depends_on - ignoring        
{
  "kind": "List",
  "apiVersion": "v1",
  "metadata": {},
  "items": [
    {
      "kind": "Service",
      "apiVersion": "v1",

cdrage · 2016-11-24T14:36:41Z

@surajssd thanks! so it looks like an issue reading the env's, i'm going to update the PR and see what happens.

edit: how I feel right now trying to get these tests to pass:

opened up issue #309 as I'm getting a weird error running the cmd tests on my local machine.

surajssd · 2016-11-24T14:45:29Z

@cdrage also -y is not working for some reason

$ kompose convert --stdout -y
Error: unknown shorthand flag: 'y' in -y
<snip>

cdrage · 2016-11-24T14:50:08Z

@surajssd updated, should work now. added -y to yaml. had forgotten to add it.

cdrage · 2016-11-25T16:02:00Z

@surajssd @kadel I've been trying to resolve the test scenarios but I'm having issues making them pass :( Even though the output is the exact same as before this PR, it doesn't seem to "match" correctly. Any ideas?

cdrage · 2016-11-28T13:59:57Z

For example: Here's a test that should pass even after doing export $(cat envs). It's the desired output:

test/fixtures/etherpad  switch-to-cobra ✔                                                                                                                                                                                                                                    3d  
▶ export $(cat envs)                                                                                                                                   

test/fixtures/etherpad  switch-to-cobra ✔                                                                                                                                                                                                                                    3d  
▶ kompose --provider=openshift -f $GOPATH/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/etherpad/docker-compose.yml convert --stdout
WARN[0000] Unsupported key depends_on - ignoring        
{
  "kind": "List",
  "apiVersion": "v1",
  "metadata": {},
  "items": [
    {

surajssd · 2016-11-29T15:31:35Z

@cdrage so looking closely it seems it's generating deployment and deploymentconfigs both, so with openshift we only generate deploymentconfig as default controller object.

cdrage · 2016-11-29T16:46:29Z

@surajssd

I see, so in: https://github.com/kubernetes-incubator/kompose/blob/6033025c058e44f00d78c42149a09be025ba6a31/cli/command/command.go

We actually have different flags depending on the provider (which I think is a big no-no since suddenly flags disappear if a different provider is provided).

So what essentially happens is that by default "deployment" is false since it's not implied. Got it. I'll fix this :)

cdrage · 2016-11-29T16:51:35Z

If anyone else has a different opinion / way to implement, that'd be great.

Another option for "splitting" the flags is simply having a validation (if you pass in --chart for OpenShift it'll simply error out as not compatible / usable).

surajssd · 2016-11-29T18:01:46Z

@cdrage that was purposeful changing controller flags depending on provider user mentions.

cdrage · 2016-11-29T18:24:16Z

All tests pass now 👍

surajssd · 2016-12-01T07:35:15Z

cmd/convert.go

+
+func init() {
+
+	convertCmd.Flags().BoolVarP(&ConvertChart, "chart", "c", false, "Create a Helm chart for converted objects **Kubernetes only**")


IMO having ** in help is making it look cluttered on command line.

surajssd · 2016-12-01T07:40:42Z

@cdrage I see flags which are not for provider openshift ? It will confuse users actually, this was the argument behind changing flags on changing providers. Ref: #182 (comment)

$ ./kompose --provider openshift convert --help
A longer description that spans multiple lines and likely contains examples

Usage:
  kompose convert [file] [flags]

Flags:
  -c, --chart                   Create a Helm chart for converted objects **Kubernetes only**
      --daemonset               Generate a Kubernetes daemonset object **Kubernetes only**
  -d, --deployment              Generate a Kubernetes deployment object **OpenShift only**
      --deploymentconfig        Generate a deployment config object. **OpenShift only*
      --emptyvols               Use Empty Volumes. Do not generate PVCs
  -o, --out string              Specify a file name to save objects to
      --replicas int            Specify the number of repliaces in the generate resource spec (default 1)
      --replicationcontroller   Generate a Kubernetes replication controller object **Kubernetes only**
      --stdout                  Print converted objects to stdout
  -y, --yaml                    Generate resource files into yaml format

Global Flags:
  -b, --bundle string       Specify a Distributed Application GlobalBundle (DAB) file
      --error-on-warning    Treat any warning as an error
  -f, --file string         Specify an alternative compose file (default "docker-compose.yml")
      --provider string     Specify a provider. Kubernetes or OpenShift. (default "kubernetes")
      --suppress-warnings   Suppress all warnings
  -v, --verbose             verbose output

surajssd · 2016-12-01T11:16:05Z

following do not work

kompose convert --ds
kompose convert --rc
kompose --provider openshift convert --dc

kompose --dab file convert

following should error out but it still continues execution

kompose --provider openshift convert -d

You can set provider using env var PROVIDER but now it is not taking effect. Similarly for dab file and compose file and output file.

Good job with having replicas under sub-command up.

cdrage · 2016-12-01T13:13:26Z

@surajssd

kompose convert --ds
kompose convert --rc
kompose --provider openshift convert --dc

kompose --dab file convert

Those errors are intentional...

-rc, etc is unconventional for CLI. it should either be a full name --replicationcontroller or one letter, ex. -r.

kompose --provider openshift convert -d

For this I will fix ^^.

From your comment, I will split off the CLI flags based on provider so they do not appear. Thanks for another review!

coveralls · 2016-12-19T15:17:25Z

Coverage increased (+4.4%) to 40.332% when pulling 6e736bf on cdrage:switch-to-cobra into 072d481 on kubernetes-incubator:master.

cdrage · 2016-12-19T15:26:08Z

@surajssd updated with vendoring :)

surajssd · 2016-12-19T15:39:21Z

$ kompose convert --deployment-config --stdout worked without errors, it created deployment and services.

@cdrage with this I mean that this should error out, here provider is considered as kubernetes and then when i say deployment-config this should error out because those are conflicting flags.

cdrage · 2016-12-19T15:46:16Z

@surajssd Yeah, because of @sebgoa suggestions to put it into one Resources section, I was unable to differentiate them / error out based on provider.

I'll have to implement a custom solution.

We're you able to test the environment variable changes? Testing passes so I'm assuming env $(export envs) works with environment variables + kompose.

surajssd · 2016-12-19T16:59:40Z

Btw, I added one line of code viper.AutomaticEnvs for grabbing environment variables automatically. Tests seems to pass correctly (even before adding this) despite doing an import of variables with env $(export foobar).

@cdrage so this is I guess for cobra flags that can be replaced with env vars, but the thing about env vars you mentioned are read by libcompose to replace in docker-compose file we provide.
I hope I answered your question!

surajssd · 2016-12-19T17:52:02Z

I'll have to implement a custom solution.

func ValidateFlags can have that logic I guess!

cdrage · 2016-12-19T19:23:41Z

@surajssd Ahhhh. that makes sense.. So right now it doesn't seem to work when doing PROVIDER=openshift kompose convert ?

cdrage · 2016-12-20T17:10:23Z

@surajssd @sebgoa @ngtuna

I've gone ahead and done (hopefully) the last code change to this PR.

This is ready for another review.

I've gone ahead and fixed @surajssd suggestions in regards to the flag validation against each provider.

For example:

▶ kompose --provider=kubernetes convert --deployment-config --stdout
FATA[0000] --deployment-config is an OpenShift only flag 

▶ kompose --provider=openshift convert --deployment-config --chart --stdout
FATA[0000] --chart, -c is a Kubernetes only flag

coveralls · 2016-12-20T17:14:32Z

Coverage increased (+7.2%) to 43.093% when pulling 7020e10 on cdrage:switch-to-cobra into 072d481 on kubernetes-incubator:master.

coveralls · 2016-12-21T14:04:06Z

Coverage increased (+3.9%) to 44.691% when pulling 9f6ea1e on cdrage:switch-to-cobra into 48e3ba8 on kubernetes-incubator:master.

surajssd · 2016-12-21T18:26:22Z

pkg/app/app.go

+			logrus.Fatalf("--deployment-config is an OpenShift only flag")
+		}
+		if deployment {
+			logrus.Fatalf("--deployment, -d is an OpenShift only flag")


It's a kubernetes only flag.

coveralls · 2016-12-21T21:37:45Z

Coverage increased (+3.9%) to 44.691% when pulling 39f2a1c on cdrage:switch-to-cobra into 48e3ba8 on kubernetes-incubator:master.

cdrage · 2016-12-21T21:48:31Z

thanks @surajssd I've updated it. Can't believe I missed that!

coveralls · 2016-12-21T21:56:29Z

Coverage increased (+3.9%) to 44.691% when pulling 58fe43c on cdrage:switch-to-cobra into 48e3ba8 on kubernetes-incubator:master.

There's A LOT happening in this commit, so here's an outline: First off, urfave/cli has been removed in favour of spf13/cobra. With this, comes changes to the formatting as well as the help page for Kompose. Upon converting, I noticed a CLI flag was NOT appearing for OpenShift. Specifically, --deploymentconfig. This has been added with a note that says it is OpenShift only. Exit codes have been fixed. If the conversion / down / up fails for any reason, Kompose will exit with Code 1. --verbose as well as --suppress-warnings can now be set at the same time. app_test.go in the cli directory has been moved to pkg/transformer to better reflect the testing coverage. version.go has been removed and converted to it's own CLI command in conjuction with (most) Go software. A new CLI command has been created. kompose version --dab isn't a conventional way for short-form CLI paramters. This has been shortened to -b for bundle. CLI flags consisting of only two/three letters have been removed due to it being unconventional for CLI. For example, --dc was removed in preference for --deploymentconfig --replicas has been added as an option when using kompose down or kompose up. This has been added as previously in app.go the replica amount was hard-coded as 1. Differentiating names have been used for flags. For example, persistent flags use the name Global (ex. GlobalOut). Command-specific flags have their own names (ex. UpOpt). Closes kubernetes#239 kubernetes#253

cdrage · 2016-12-22T13:16:43Z

Hey @surajssd

~~I keep having merge conflicts daily with adding vendoring to this PR. I'm going to create a separate PR for this for when we merge this PR so there's no conflicts 👍~~

Nevermind, travis won't build unless I add the vendoring.

coveralls · 2016-12-22T14:25:45Z

Coverage increased (+3.7%) to 45.739% when pulling 1fdc1f6 on cdrage:switch-to-cobra into 240f150 on kubernetes-incubator:master.

rtnpro · 2016-12-23T13:39:06Z

LGTM 👍

surajssd · 2016-12-26T07:03:37Z

@cdrage thanks for awesome work ! Merging this! @rtnpro thanks for giving it a look.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2016

cdrage force-pushed the switch-to-cobra branch from bf0e17d to ea1581f Compare November 22, 2016 13:53

cdrage force-pushed the switch-to-cobra branch from ea1581f to 34d78ac Compare November 22, 2016 18:37

cdrage force-pushed the switch-to-cobra branch 2 times, most recently from 21a6194 to 7aa894c Compare November 23, 2016 17:00

cdrage force-pushed the switch-to-cobra branch from 7aa894c to c8316e7 Compare November 24, 2016 14:49

cdrage mentioned this pull request Nov 24, 2016

Default to YAML output #306

Closed

kadel mentioned this pull request Nov 25, 2016

added support for multiple-compose files #312

Merged

cdrage force-pushed the switch-to-cobra branch from c8316e7 to 2447483 Compare November 25, 2016 16:02

sebgoa mentioned this pull request Nov 29, 2016

Issues regarding CLI. Perhaps switching to Cobra? #253

Closed

cdrage force-pushed the switch-to-cobra branch from 2447483 to 303ebf7 Compare November 29, 2016 17:13

surajssd reviewed Dec 1, 2016

View reviewed changes

cdrage force-pushed the switch-to-cobra branch from 9317f10 to 9aa3edf Compare December 19, 2016 14:34

cdrage force-pushed the switch-to-cobra branch from 7020e10 to 9f6ea1e Compare December 21, 2016 13:56

surajssd reviewed Dec 21, 2016

View reviewed changes

cdrage force-pushed the switch-to-cobra branch from 9f6ea1e to 39f2a1c Compare December 21, 2016 21:29

cdrage force-pushed the switch-to-cobra branch from 39f2a1c to 1134e51 Compare December 21, 2016 21:41

cdrage force-pushed the switch-to-cobra branch from 58fe43c to 9f6cd56 Compare December 22, 2016 13:15

cdrage force-pushed the switch-to-cobra branch from 9f6cd56 to 1b9228e Compare December 22, 2016 13:15

Update vendoring

1fdc1f6

surajssd approved these changes Dec 26, 2016

View reviewed changes

surajssd merged commit 7341831 into kubernetes:master Dec 26, 2016

cdrage deleted the switch-to-cobra branch March 30, 2017 13:09

cflee mentioned this pull request Oct 4, 2019

CLI usage message does not mention ability to set --out directory #1170

Closed


		func init() {

		convertCmd.Flags().BoolVarP(&ConvertChart, "chart", "c", false, "Create a Helm chart for converted objects Kubernetes only")

Switch to spf13/cobra from urfave/cli #304

Switch to spf13/cobra from urfave/cli #304

Conversation

cdrage commented Nov 21, 2016 • edited Loading

cdrage commented Nov 21, 2016

cdrage commented Nov 22, 2016 • edited Loading

surajssd commented Nov 23, 2016

cdrage commented Nov 23, 2016 • edited Loading

surajssd commented Nov 24, 2016

cdrage commented Nov 24, 2016 • edited Loading

surajssd commented Nov 24, 2016

cdrage commented Nov 24, 2016

cdrage commented Nov 25, 2016

cdrage commented Nov 28, 2016

surajssd commented Nov 29, 2016

cdrage commented Nov 29, 2016

cdrage commented Nov 29, 2016

surajssd commented Nov 29, 2016

cdrage commented Nov 29, 2016

surajssd Dec 1, 2016

Choose a reason for hiding this comment

surajssd commented Dec 1, 2016

surajssd commented Dec 1, 2016

cdrage commented Dec 1, 2016

coveralls commented Dec 19, 2016

cdrage commented Dec 19, 2016

surajssd commented Dec 19, 2016

cdrage commented Dec 19, 2016

surajssd commented Dec 19, 2016 • edited Loading

surajssd commented Dec 19, 2016

cdrage commented Dec 19, 2016

cdrage commented Dec 20, 2016

coveralls commented Dec 20, 2016

coveralls commented Dec 21, 2016

surajssd Dec 21, 2016

Choose a reason for hiding this comment

coveralls commented Dec 21, 2016

cdrage commented Dec 21, 2016

coveralls commented Dec 21, 2016

cdrage commented Dec 22, 2016 • edited Loading

coveralls commented Dec 22, 2016

rtnpro commented Dec 23, 2016

surajssd commented Dec 26, 2016

cdrage commented Nov 21, 2016 •

edited

Loading

cdrage commented Nov 22, 2016 •

edited

Loading

cdrage commented Nov 23, 2016 •

edited

Loading

cdrage commented Nov 24, 2016 •

edited

Loading

surajssd commented Dec 19, 2016 •

edited

Loading

cdrage commented Dec 22, 2016 •

edited

Loading