make YAML the default kompose conversion #339

procrypt · 2016-12-16T10:44:45Z

$ kompose convert
INFO[0000] file "mlbparks-service.yaml" created         
INFO[0000] file "mongodb-service.yaml" created          
INFO[0000] file "mlbparks-deployment.yaml" created      
INFO[0000] file "mongodb-deployment.yaml" created       
INFO[0000] file "mongodb-claim0-persistentvolumeclaim.yaml" created

$ kompose convert -j
INFO[0000] file "mlbparks-service.json" created         
INFO[0000] file "mongodb-service.json" created          
INFO[0000] file "mlbparks-deployment.json" created      
INFO[0000] file "mongodb-deployment.json" created       
INFO[0000] file "mongodb-claim0-persistentvolumeclaim.json" created

cdrage · 2016-12-16T14:07:44Z

Still waiting for #304 to be merged too :(

procrypt · 2016-12-17T04:45:43Z

@cdrage How long do we have to wait for #304 to get merged. I have to update this PR once #304 gets merged :)

cdrage · 2016-12-19T14:38:06Z

@procrypt no idea, depending on the reviews, if you can review it too + test it, that'll speed up the process :)

procrypt · 2016-12-19T14:50:15Z

@cdrage I'm on it 😃

surajssd · 2016-12-20T02:11:58Z

@procrypt so we don't remove a flag, we will add another flag for json output, default output to yaml and also there is a way in cmdline framework to mention what flag is gonna be deprecated. Remove flag of yaml right away might break things.

surajssd · 2016-12-28T07:48:36Z

pkg/app/app.go

@@ -163,6 +164,7 @@ func validateControllers(opt *kobject.ConvertOptions) {
 }

 // Convert transforms docker compose or dab file to k8s objects
+


excess space between function and its definition

procrypt · 2016-12-28T11:35:02Z

@surajssd @containscafeine @cdrage @pradeepto I have updated the PR. Please review.

$ kompose convert
INFO[0000] file "mlbparks-service.yaml" created         
INFO[0000] file "mongodb-service.yaml" created          
INFO[0000] file "mlbparks-deployment.yaml" created      
INFO[0000] file "mongodb-deployment.yaml" created       
INFO[0000] file "mongodb-claim0-persistentvolumeclaim.yaml" created

$kompose convert -j
Flag --json has been deprecated, use --yaml or -y
INFO[0000] file "mlbparks-service.json" created         
INFO[0000] file "mongodb-service.json" created          
INFO[0000] file "mlbparks-deployment.json" created      
INFO[0000] file "mongodb-deployment.json" created       
INFO[0000] file "mongodb-claim0-persistentvolumeclaim.json" created

coveralls · 2016-12-28T11:40:50Z

Coverage remained the same at 45.682% when pulling fa36f31 on procrypt:default_yaml into c80735c on kubernetes-incubator:master.

surajssd · 2016-12-28T12:14:36Z

cmd/convert.go

@@ -87,6 +88,9 @@ func init() {

 	// Standard between the two
 	convertCmd.Flags().BoolVarP(&ConvertYaml, "yaml", "y", false, "Generate resource files into yaml format")
+	convertCmd.Flags().BoolVarP(&ConvertJson, "json", "j", false, "Generate resource files into yaml format")
+	convertCmd.Flags().MarkDeprecated("json", "use --yaml or -y")


We are deprecating -y right?

Right and thanks for the catch, I depreciated the wrong flag.

surajssd · 2016-12-28T12:17:37Z

@procrypt for making tests work you will have to add -j flag in all the cmd tests. Right now it creates default json so there was no flag, with your changes you will have to add that to all the tests.

procrypt · 2016-12-29T05:02:58Z

@surajssd Since we want the default conversion to be in yaml and the output of all the tests is in json format. I was thinking to replace all the test output from json to yaml, instead of adding the -j flag.
WDYT ?

coveralls · 2016-12-29T10:31:53Z

Coverage remained the same at 46.582% when pulling b102e1a on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2016-12-29T10:45:44Z

Coverage remained the same at 46.582% when pulling fe800ab on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2016-12-29T10:48:58Z

Coverage remained the same at 46.582% when pulling fe800ab on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

surajssd · 2017-01-02T06:37:38Z

cmd/convert.go

@@ -46,6 +46,7 @@ var convertCmd = &cobra.Command{
 			ToStdout:               ConvertStdout,
 			CreateChart:            ConvertChart,
 			GenerateYaml:           ConvertYaml,
+			GenerateJson:           ConvertJson,


I think we can get rid of ConvertYaml in code everywhere, since we are not checking on it anymore.

After doing some iterations on trying to get rid of ConvertYaml it feels like it's only making code more convoluted in terms of making checks and validation, I think we can get rid of ConvertYaml once we decide we wanna get rid of the flag.

@surajssd feels cleaner to have both, right? json + yaml generation (by default, you can set the value of GenerateYaml to true in cobra)

surajssd · 2017-01-02T06:38:41Z

cmd/convert.go

-	ConvertOpt                                                      kobject.ConvertOptions
+	ConvertSource, ConvertOut, ConvertBuildRepo, ConvertBuildBranch       string
+	ConvertChart, ConvertDeployment, ConvertDaemonSet                     bool
+	ConvertReplicationController, ConvertYaml, ConvertStdout, ConvertJson bool


same as https://github.com/kubernetes-incubator/kompose/pull/339/files#r94295029

surajssd · 2017-01-02T06:38:53Z

pkg/kobject/kobject.go

@@ -37,6 +37,7 @@ type ConvertOptions struct {
 	BuildBranch            string
 	CreateChart            bool
 	GenerateYaml           bool


same as https://github.com/kubernetes-incubator/kompose/pull/339/files#r94295029

surajssd · 2017-01-02T06:41:51Z

cmd/convert.go

@@ -93,6 +94,9 @@ func init() {

 	// Standard between the two
 	convertCmd.Flags().BoolVarP(&ConvertYaml, "yaml", "y", false, "Generate resource files into yaml format")


once convertyaml is removed this should still be functional for sometime, so if this flag is set what can be done here is set ConvertJson to be false.

surajssd · 2017-01-02T06:43:12Z

Just to be sure check if user is not giving -y and -j simultaneously.

procrypt · 2017-01-02T14:22:33Z

@surajssd Added the check for not providing -y and -j simultaneously.

coveralls · 2017-01-02T14:29:20Z

Coverage remained the same at 46.582% when pulling 88c87a3 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T04:22:18Z

Coverage remained the same at 46.582% when pulling dddc533 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T07:23:14Z

Coverage remained the same at 46.582% when pulling 92bb3b8 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T07:27:10Z

Coverage remained the same at 46.582% when pulling 92bb3b8 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T07:38:17Z

Coverage remained the same at 46.582% when pulling d8ae85a on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T07:43:18Z

Coverage remained the same at 46.582% when pulling 6c5174b on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T09:40:20Z

Coverage remained the same at 46.582% when pulling eb08a34 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T09:50:19Z

Coverage remained the same at 46.582% when pulling a1af9dc on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T10:11:02Z

Coverage remained the same at 46.582% when pulling 6995a69 on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

coveralls · 2017-01-03T11:51:03Z

Coverage remained the same at 46.582% when pulling f49b93d on procrypt:default_yaml into 9c3fdaa on kubernetes-incubator:master.

surajssd · 2017-01-03T11:56:10Z

@procrypt code LGTM, one last bit of update docs: https://github.com/kubernetes-incubator/kompose/blob/master/docs/user-guide.md and https://github.com/kubernetes-incubator/kompose#use-case

coveralls · 2017-01-03T13:27:35Z

Coverage remained the same at 46.617% when pulling 4ab3303 on procrypt:default_yaml into b059c44 on kubernetes-incubator:master.

cdrage · 2017-01-03T18:47:33Z

@procrypt Tests need to be converted to check against yaml instead.

cdrage · 2017-01-03T18:48:33Z

cmd/convert.go

@@ -46,6 +46,7 @@ var convertCmd = &cobra.Command{
 			ToStdout:               ConvertStdout,
 			CreateChart:            ConvertChart,
 			GenerateYaml:           ConvertYaml,
+			GenerateJson:           ConvertJson,


@surajssd feels cleaner to have both, right? json + yaml generation (by default, you can set the value of GenerateYaml to true in cobra)

cdrage · 2017-01-03T18:49:24Z

cmd/convert.go

@@ -93,6 +94,9 @@ func init() {

 	// Standard between the two
 	convertCmd.Flags().BoolVarP(&ConvertYaml, "yaml", "y", false, "Generate resource files into yaml format")
+	convertCmd.Flags().MarkDeprecated("yaml", "YAML is the default format now.")
+	convertCmd.Flags().MarkShorthandDeprecated("y", "YAML is the default format now.")
+	convertCmd.Flags().BoolVarP(&ConvertJson, "json", "j", false, "Generate resource files into json format")


JSON should be capitalized

cdrage · 2017-01-03T18:51:12Z

pkg/app/app.go

@@ -124,6 +124,10 @@ func ValidateFlags(bundle string, args []string, cmd *cobra.Command, opt *kobjec
 	if len(args) != 0 {
 		logrus.Fatal("Unknown Argument(s): ", strings.Join(args, ","))
 	}
+
+	if opt.GenerateJson && opt.GenerateYaml {
+		logrus.Fatalf("Error: 'YAML' and 'JSON' format cannot be provided at the same time")


No need to add Error:. and IMO YAML and JSON shouldn't be in quotes.

coveralls · 2017-01-04T04:47:32Z

Coverage remained the same at 46.617% when pulling d17edbb on procrypt:default_yaml into b059c44 on kubernetes-incubator:master.

coveralls · 2017-01-04T07:29:07Z

Coverage remained the same at 46.617% when pulling cfcbfa8 on procrypt:default_yaml into b059c44 on kubernetes-incubator:master.

surajssd · 2017-01-04T18:03:54Z

@procrypt thanks for awesome work 👍 🎉

cdrage · 2017-01-04T18:05:44Z

thanks @procrypt 👍

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

procrypt force-pushed the default_yaml branch 2 times, most recently from 1b2f821 to 711d914 Compare December 27, 2016 09:39

surajssd reviewed Dec 28, 2016

View reviewed changes

procrypt force-pushed the default_yaml branch from 711d914 to fa36f31 Compare December 28, 2016 11:32

surajssd reviewed Dec 28, 2016

View reviewed changes

procrypt force-pushed the default_yaml branch 2 times, most recently from 8d2c92b to b102e1a Compare December 29, 2016 10:24

procrypt force-pushed the default_yaml branch 2 times, most recently from 9b7ea58 to fe800ab Compare December 29, 2016 10:41

surajssd reviewed Jan 2, 2017

View reviewed changes

procrypt force-pushed the default_yaml branch from fe800ab to 88c87a3 Compare January 2, 2017 14:21

procrypt force-pushed the default_yaml branch from 88c87a3 to dddc533 Compare January 3, 2017 04:14

procrypt force-pushed the default_yaml branch from e54f9e0 to 92bb3b8 Compare January 3, 2017 07:18

procrypt force-pushed the default_yaml branch 2 times, most recently from d8ae85a to 6c5174b Compare January 3, 2017 07:35

procrypt force-pushed the default_yaml branch from 6c5174b to eb08a34 Compare January 3, 2017 09:32

procrypt force-pushed the default_yaml branch from eb08a34 to a1af9dc Compare January 3, 2017 09:41

procrypt force-pushed the default_yaml branch from a1af9dc to 6995a69 Compare January 3, 2017 10:03

procrypt force-pushed the default_yaml branch from 6995a69 to f49b93d Compare January 3, 2017 11:42

procrypt force-pushed the default_yaml branch from f49b93d to 4ab3303 Compare January 3, 2017 13:20

cdrage requested changes Jan 3, 2017

View reviewed changes

procrypt force-pushed the default_yaml branch from 4ab3303 to d17edbb Compare January 4, 2017 04:39

make default kompose conversion YAML

cfcbfa8

procrypt force-pushed the default_yaml branch from d17edbb to cfcbfa8 Compare January 4, 2017 07:21

surajssd approved these changes Jan 4, 2017

View reviewed changes

surajssd added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2017

surajssd merged commit a14d5c2 into kubernetes:master Jan 4, 2017

		@@ -163,6 +164,7 @@ func validateControllers(opt *kobject.ConvertOptions) {
		}

		// Convert transforms docker compose or dab file to k8s objects

		@@ -93,6 +94,9 @@ func init() {

		// Standard between the two
		convertCmd.Flags().BoolVarP(&ConvertYaml, "yaml", "y", false, "Generate resource files into yaml format")

make YAML the default kompose conversion #339

make YAML the default kompose conversion #339

Conversation

procrypt commented Dec 16, 2016 • edited Loading

cdrage commented Dec 16, 2016

procrypt commented Dec 17, 2016

cdrage commented Dec 19, 2016

procrypt commented Dec 19, 2016

surajssd commented Dec 20, 2016

Choose a reason for hiding this comment

procrypt commented Dec 28, 2016 • edited Loading

coveralls commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Dec 28, 2016

procrypt commented Dec 29, 2016

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

coveralls commented Dec 29, 2016

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

surajssd commented Jan 2, 2017

procrypt commented Jan 2, 2017 • edited Loading

coveralls commented Jan 2, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

coveralls commented Jan 3, 2017

surajssd commented Jan 3, 2017

coveralls commented Jan 3, 2017

cdrage commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 4, 2017

coveralls commented Jan 4, 2017

surajssd commented Jan 4, 2017

cdrage commented Jan 4, 2017

procrypt commented Dec 16, 2016 •

edited

Loading

procrypt commented Dec 28, 2016 •

edited

Loading

procrypt commented Jan 2, 2017 •

edited

Loading