Add JSON and YAML output to svcat #1944

jeremyrickard · 2018-04-16T23:02:40Z

Adding JSON and YAML output capabilities

Note: This is not 100% complete. Right now, we do not obtain the
metav1.TypeMeta within svcat. We will need to figure out how to
populate this.

See: kubernetes/client-go#308 (comment)

Fixes: #1814

jeremyrickard · 2018-04-16T23:13:41Z

Example output

$ ./bin/svcat/svcat get instance cosmosdb -o json
{
   "metadata": {
      "name": "cosmosdb",
      "namespace": "default",
      "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/namespaces/default/serviceinstances/cosmosdb",
      "uid": "dffb52b5-41a1-11e8-8f2d-0a580af4010c",
      "resourceVersion": "432",
      "generation": 1,
      "creationTimestamp": "2018-04-16T18:13:33Z",
      "finalizers": [
         "kubernetes-incubator/service-catalog"
      ]
   },
   "spec": {
      "clusterServiceClassExternalName": "azure-cosmosdb-mongo-account",
      "clusterServicePlanExternalName": "account",
      "clusterServiceClassRef": {
         "name": "8797a079-5346-4e84-8018-b7d5ea5c0e3a"
      },
      "clusterServicePlanRef": {
         "name": "86fdda05-78d7-4026-a443-1325928e7b02"
      },
      "parameters": {
         "location": "eastus"
      },
      "externalID": "dffb4fd6-41a1-11e8-8f2d-0a580af4010c",
      "updateRequests": 0
   },
   "status": {
      "conditions": [
         {
            "type": "Ready",
            "status": "True",
            "lastTransitionTime": "2018-04-16T18:17:50Z",
            "reason": "ProvisionedSuccessfully",
            "message": "The instance was provisioned successfully"
         }
      ],
      "asyncOpInProgress": false,
      "orphanMitigationInProgress": false,
      "reconciledGeneration": 1,
      "observedGeneration": 1,
      "externalProperties": {
         "clusterServicePlanExternalName": "account",
         "clusterServicePlanExternalID": "86fdda05-78d7-4026-a443-1325928e7b02",
         "parameters": {
            "location": "eastus"
         },
         "parameterChecksum": "d0ec58f6385d6c93695c4a23b5f3c7c81c91f31e0f8502f3189cf4a44e7d3095"
      },
      "provisionStatus": "Provisioned",
      "deprovisionStatus": "Required"
   }
}

carolynvs

THIS MAKES ME SO HAPPY! 💃

Don't be overwhelmed by the # of my comments, looks great and with a few tweaks will be ready to rock.

carolynvs · 2018-04-16T23:17:34Z

cmd/svcat/binding/get_cmd.go

@@ -45,7 +50,7 @@ func NewGetCmd(cxt *command.Context) *cobra.Command {
 	}

 	command.AddNamespaceFlags(cmd.Flags(), true)
-
+	command.AddOutputFlags(cmd.Flags())


Because the --output flag is used by kubectl as well, you will need to add an explicit mapping for when svcat is used in plugin mode.

See https://github.com/kubernetes-incubator/service-catalog/blob/2e1ca7f8cf2b5d45b0542602f0d12cb18bb05f98/cmd/svcat/plugin/plugin.go#L60-L66 for an example of how we do that with the -v and --namespace flags.

Does that need to happen for "--output" and "-o"? Or is it smart enough to handle that?

carolynvs · 2018-04-16T23:19:21Z

cmd/svcat/binding/get_cmd.go

@@ -71,7 +76,7 @@ func (c *getCmd) getAll() error {
 		return err
 	}

-	output.WriteBindingList(c.Output, bindings.Items...)
+	output.WriteBindingList(c.Output, c.outputFormat, bindings)


👍 Yeah I get what you were saying before now about needing to switch this to use the root object and not just items.

carolynvs · 2018-04-16T23:21:39Z

cmd/svcat/class/get_cmd.go

@@ -81,7 +87,7 @@ func (c *getCmd) getAll() error {
 		return err
 	}

-	output.WriteClassList(c.Output, classes...)
+	output.WriteClassList(c.Output, c.outputFormat, classes...)


This is my mistake. c.App.RetrieveClasses() should return the entire type and not just items, like bindings does. There may be a few other types that should standardize as well on returning the entire response and not just items but that is fine (probably best) to be a follow-on PR.

What do you think?

carolynvs · 2018-04-16T23:24:04Z

cmd/svcat/command/command.go

+	format = strings.ToLower(format)
+
+	switch format {
+	case "", "table":


Just wanted to agree that handling "" and "table" here instead of setting the default value on the flag is the correct thing to do because in plugin mode, the flag's default value isn't under our control. 👍

carolynvs · 2018-04-16T23:25:05Z

cmd/svcat/command/command.go

+	case "yaml":
+		return "yaml", nil
+	default:
+		return "", fmt.Errorf("unknown output format: %s", format)


I think it would help to mention the flag and acceptable formats, maybe something like this?

invalid --output format %q, allowed values are table, json and yaml

carolynvs · 2018-04-16T23:37:15Z

cmd/svcat/plan/get_cmd.go

 	// Retrieve the class as well because plans don't have the external class name
 	class, err := c.App.RetrieveClassByID(plan.Spec.ClusterServiceClassRef.Name)
 	if err != nil {
 		return err
 	}

-	output.WritePlanList(c.Output, []v1beta1.ClusterServicePlan{*plan}, []v1beta1.ClusterServiceClass{*class})
+	output.WritePlan(c.Output, c.outputFormat, *plan, *class)


carolynvs · 2018-04-16T23:39:27Z

cmd/svcat/testdata/output/get-plans.yaml

@@ -0,0 +1,129 @@
+{


haha this isn't yaml 😀

carolynvs · 2018-04-16T23:42:52Z

cmd/svcat/svcat_test.go

 		{name: "get class by name", cmd: "get class user-provided-service", golden: "output/get-class.txt"},
 		{name: "get class by uuid", cmd: "get class --uuid 4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468", golden: "output/get-class.txt"},
 		{name: "describe class by name", cmd: "describe class user-provided-service", golden: "output/describe-class.txt"},
 		{name: "describe class uuid", cmd: "describe class --uuid 4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468", golden: "output/describe-class.txt"},

 		{name: "list all plans", cmd: "get plans", golden: "output/get-plans.txt"},
+		{name: "list all plans (json)", cmd: "get plans -o json", golden: "output/get-plans.json"},
+		{name: "list all plans (yaml)", cmd: "get plans -o yaml", golden: "output/get-plans.yaml"},
 		{name: "get plan by name", cmd: "get plan default", golden: "output/get-plan.txt"},


There is a missing testcase for outputting yaml/json for "get plan by name". No need to do it for uuid too.

carolynvs · 2018-04-16T23:43:41Z

cmd/svcat/svcat_test.go

 		{name: "describe broker", cmd: "describe broker ups-broker", golden: "output/describe-broker.txt"},

 		{name: "list all classes", cmd: "get classes", golden: "output/get-classes.txt"},
+		{name: "list all classes (json)", cmd: "get classes -o json", golden: "output/get-classes.json"},
+		{name: "list all classes (yaml)", cmd: "get classes -o yaml", golden: "output/get-classes.yaml"},
 		{name: "get class by name", cmd: "get class user-provided-service", golden: "output/get-class.txt"},


There is a missing testcase for outputting yaml/json for "get class by name". No need to do it for uuid too.

carolynvs · 2018-04-16T23:47:40Z

cmd/svcat/testdata/plugin.yaml

@@ -88,13 +88,26 @@ tree:
    - name: all-namespaces
      desc: If present, list the requested object(s) across all namespaces. Namespace
        in current context is ignored even if specified with --namespace
+    - name: output


Oh gosh, I just realized that the doc I added to the dev guide explaining how to easily update the golden files went missing! 😝 I hope that didn't waste too much of your time.

The trick is this (i'll submit a PR in a minute to fix the doc)

touch cmd/svcat/testdata/output/NEWFILE

go test ./cmd/svcat/... -update

That will insert the test output into the golden file without you having to do it manually.

carolynvs

LGTM once the changes to the plugin file are reverted. Sorry for the wrong advice earlier!

carolynvs · 2018-04-18T13:40:19Z

cmd/svcat/output/binding.go

-	case "yaml":
-		writeBindingListYAML(w, bindingList)
-	case "table":
+	case formatJSON:


This is sooooooooo much better!

carolynvs · 2018-04-18T13:43:18Z

cmd/svcat/output/json.go

+
+func writeJSON(w io.Writer, obj interface{}, n int) {
+
+	if n == 0 {


Ignoring the value they set seems a bit unexpected to me. If it's a *int then checking null would be less surprising than converting 0 -> 3. Though then it makes it hard to pass in an int... 🤔

Just something to think about, we can tweak later.

For now, I just went ahead and set it to do three spaces. If we run into the need for variable indent on JSON output, let's revisit?

carolynvs · 2018-04-18T13:47:33Z

cmd/svcat/plugin/plugin.go

@@ -46,6 +46,9 @@ const (
 	// EnvPluginVerbose is the -v=LEVEL flag
 	EnvPluginVerbose = EnvPluginGlobalFlagPrefix + "_V"

+	// EnvPluginOutput is the -o=FORMAT flag


I just tested this in plugin mode and I apologize for sending you down the wrong path. It won't be sent to us as a global flag after all so the changes to this file can be reverted. Our default plugin wiring will handle setting it without this (because it will come in as an env var like this KUBECTL_PLUGINS_LOCAL_FLAG_OUTPUT=json).

This adds JSON/YAML output to svcat, but does not currently include the Type Metadata, so we lose the Kind/GroupVersion info.

Note: This is not 100% complete. Right now, we do not obtain the metav1.TypeMeta within svcat. We will need to figure out how to populate this. See: kubernetes/client-go#308 (comment) Fixes: kubernetes-retired#1814

jeremyrickard · 2018-04-19T00:02:16Z

@carolynvs ok! take a look again, all the changes we discussed should be done now!

carolynvs

LGTM

Can't wait to use this! 💯

n3wscott · 2018-04-19T16:34:21Z

Awesome work.

MHBauer · 2018-04-19T17:40:55Z

At the top it says Note: This is not 100% complete. Does it need to be complete to merge? Is this PR ready to merge now?

carolynvs · 2018-04-19T17:46:17Z

Reflecting and populating type metadata (i.e. kind) will be addressed in a follow-on since it's not really used, and would just be nice to have for completeness sake in comparison to the output of kubectl.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 16, 2018

jeremyrickard changed the title ~~Json output~~ Add JSON and YAML output to svcat Apr 16, 2018

carolynvs self-requested a review April 16, 2018 23:10

carolynvs suggested changes Apr 16, 2018

View reviewed changes

carolynvs reviewed Apr 18, 2018

View reviewed changes

jeremyrickard added 5 commits April 18, 2018 14:17

Initial WIP commit for adding JSON/YAML output to svcat

01dfea3

This adds JSON/YAML output to svcat, but does not currently include the Type Metadata, so we lose the Kind/GroupVersion info.

Adding JSON and YAML output capabilities

f72348f

Note: This is not 100% complete. Right now, we do not obtain the metav1.TypeMeta within svcat. We will need to figure out how to populate this. See: kubernetes/client-go#308 (comment) Fixes: kubernetes-retired#1814

Code review comments

3ef0ff1

Code review comments

08ed327

Added boilerplate on json.go

1e726be

jeremyrickard force-pushed the json-output branch from ecfbd53 to 1e726be Compare April 18, 2018 20:22

carolynvs approved these changes Apr 19, 2018

View reviewed changes

carolynvs added the LGTM1 label Apr 19, 2018

n3wscott added the LGTM2 label Apr 19, 2018

carolynvs merged commit 929c6e2 into kubernetes-retired:master Apr 19, 2018

carolynvs added this to the 1.0.0 milestone May 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON and YAML output to svcat #1944

Add JSON and YAML output to svcat #1944

jeremyrickard commented Apr 16, 2018

jeremyrickard commented Apr 16, 2018 •

edited

Loading

carolynvs left a comment

carolynvs Apr 16, 2018 •

edited

Loading

jeremyrickard Apr 17, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018 •

edited

Loading

jeremyrickard Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs Apr 16, 2018

carolynvs left a comment

carolynvs Apr 18, 2018

carolynvs Apr 18, 2018

jeremyrickard Apr 18, 2018

carolynvs Apr 18, 2018

jeremyrickard Apr 18, 2018

jeremyrickard commented Apr 19, 2018

carolynvs left a comment

n3wscott commented Apr 19, 2018

MHBauer commented Apr 19, 2018

carolynvs commented Apr 19, 2018


		func writeJSON(w io.Writer, obj interface{}, n int) {

		if n == 0 {

Add JSON and YAML output to svcat #1944

Add JSON and YAML output to svcat #1944

Conversation

jeremyrickard commented Apr 16, 2018

jeremyrickard commented Apr 16, 2018 • edited Loading

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs Apr 16, 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

carolynvs left a 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

jeremyrickard commented Apr 19, 2018

carolynvs left a comment

Choose a reason for hiding this comment

n3wscott commented Apr 19, 2018

MHBauer commented Apr 19, 2018

carolynvs commented Apr 19, 2018

jeremyrickard commented Apr 16, 2018 •

edited

Loading

carolynvs Apr 16, 2018 •

edited

Loading

carolynvs Apr 16, 2018 •

edited

Loading