Adds `odo catalog list services -o json` #2066

cdrage · 2019-08-30T17:29:56Z

Adds the ability to list catalog services and output to json.

The format is as follows:

{
  "kind": "ServiceList",
  "apiVersion": "odo.openshift.io/v1alpha1",
  "metadata": {
    "creationTimestamp": null
  },
  "items": [
    {
      "name": "cakephp-mysql-persistent",
      "hidden": false,
      "planList": [
        "default"
      ]
    },
  ]
}

Closes #1357

cdrage · 2019-08-30T19:29:54Z

Ready for reviewing! @dharmit @kadel @girishramnani

Unfortunately for tests, I can't test against service catalog... any ideas @amitkrout ? See my commented out test!

cdrage · 2019-09-03T17:43:08Z

/retest all

dharmit

Code looks good to me. But I feel like having the MachineReadableSuccessOutput function be a part of machineoutput package.

dharmit · 2019-09-04T05:47:46Z

pkg/service/service.go

@@ -397,3 +399,21 @@ func isRequired(required []string, name string) bool {
 	}
 	return false
 }
+
+// MachineReadableSuccessOutput prints out machine readable output
+func MachineReadableSuccessOutput(services []occlient.Service) {


Since the function is dealing with machine output, wouldn't it be helpful to keep it under the machineoutput package?

No, as each output will be different.. Or else we'll run into an issue where in the machineout package, we will have multiple functions with different names :(

For example, It'd probably be named ServiceCatalogList or something along those lines, IMO doesn't make sense. But I could be wrong.

I agree with the function name point you raised. To purely counter that point, we have adopted similar approach for auto-completion where everything lives under pkg/odo/util/completion/completionhandlers.go.

Besides that, I think, there's scope of avoiding duplication of code by putting it under machineoutput package. For example, we have renamed Success and Error structs in that package to GenericSuccess and GenericError respectively.

@kadel can you please share your thoughts?

amitkrout

Create a separate test file cmd_catalog_test.go under integration package

cdrage · 2019-09-05T20:07:11Z

You have a good point on putting the functions under machineoutout package instead, thanks @dharmit !

Ready for another review 👍

amitkrout · 2019-09-06T06:10:57Z

tests/integration/servicecatalog/cmd_service_test.go

@@ -37,6 +37,13 @@ var _ = Describe("odo service command tests", func() {
 		})
 	})

+	Context("checking machine readable output for service catalog", func() {


+1, This is a good place to validate instead of creating a separate test file.

amitkrout · 2019-09-06T06:10:59Z

tests/integration/servicecatalog/cmd_service_test.go

+	Context("checking machine readable output for service catalog", func() {
+		It("should succeed listing catalog components", func() {
+			// Since service catalog is constantly changing, we simply check to see if this command passes.. rather than checking the JSON each time.
+			helper.CmdShouldPass("odo", "catalog", "list", "services", "-o", "json")


Instead of matching the whole json can you match the basic key value pair like "kind": "ServiceList" and atleast one "items": [ you mentioned in pr description.

amitkrout · 2019-09-06T06:11:26Z

Create a separate test file cmd_catalog_test.go under integration package

#2066 (comment)

amitkrout

Remove file - tests/integration/json_test.go

cdrage · 2019-09-06T14:32:19Z

Updated based on your comments @amitkrout

amitkrout · 2019-09-06T18:27:30Z

tests/integration/servicecatalog/cmd_service_test.go

+		It("should succeed listing catalog components", func() {
+			// Since service catalog is constantly changing, we simply check to see if this command passes.. rather than checking the JSON each time.
+			output := helper.CmdShouldPass("odo", "catalog", "list", "services", "-o", "json")
+			Expect(output).To(ContainSubstring("CatalogListServices"))


amitkrout · 2019-09-06T18:30:00Z

/retest

Adds the ability to list catalog services and output to json. The format is as follows: ```json { "kind": "ServiceList", "apiVersion": "odo.openshift.io/v1alpha1", "metadata": { "creationTimestamp": null }, "items": [ { "name": "cakephp-mysql-persistent", "hidden": false, "planList": [ "default" ] }, ] } ```

amitkrout

/approve

openshift-ci-robot · 2019-09-09T05:43:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amitkrout

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [amitkrout]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

girishramnani · 2019-09-09T06:25:30Z

Looks good
/lgtm

openshift-bot · 2019-09-09T07:20:03Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/M label Aug 30, 2019

openshift-ci-robot requested review from amitkrout and girishramnani August 30, 2019 17:30

cdrage force-pushed the add-servicelist-json branch from 7d79b39 to 29791e1 Compare August 30, 2019 19:27

cdrage mentioned this pull request Aug 30, 2019

Add odo catalog list components -o json #2067

Closed

dharmit suggested changes Sep 4, 2019

View reviewed changes

amitkrout suggested changes Sep 5, 2019

View reviewed changes

cdrage force-pushed the add-servicelist-json branch from 29791e1 to 890707c Compare September 5, 2019 18:49

openshift-ci-robot added size/L and removed size/M labels Sep 5, 2019

amitkrout reviewed Sep 6, 2019

View reviewed changes

amitkrout suggested changes Sep 6, 2019

View reviewed changes

cdrage force-pushed the add-servicelist-json branch from 64fd950 to c3d9add Compare September 6, 2019 14:30

openshift-ci-robot added size/M and removed size/L labels Sep 6, 2019

cdrage force-pushed the add-servicelist-json branch from c3d9add to 4411845 Compare September 6, 2019 14:31

cdrage force-pushed the add-servicelist-json branch from 4411845 to 0782f24 Compare September 6, 2019 15:23

amitkrout reviewed Sep 6, 2019

View reviewed changes

cdrage force-pushed the add-servicelist-json branch from 0782f24 to 5dfde44 Compare September 6, 2019 18:45

amitkrout approved these changes Sep 9, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 9, 2019

openshift-ci-robot assigned girishramnani Sep 9, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 9, 2019

openshift-merge-robot merged commit 66662e4 into redhat-developer:master Sep 9, 2019

dharmit mentioned this pull request Sep 26, 2019

Bump odo version to 1.0.0-beta6 #2182

Merged

cdrage deleted the add-servicelist-json branch January 14, 2022 14:51

rm3l added the estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person label Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `odo catalog list services -o json` #2066

Adds `odo catalog list services -o json` #2066

cdrage commented Aug 30, 2019 •

edited

cdrage commented Aug 30, 2019 •

edited

cdrage commented Sep 3, 2019

dharmit left a comment

dharmit Sep 4, 2019

cdrage Sep 5, 2019

dharmit Sep 9, 2019

amitkrout left a comment •

edited

cdrage commented Sep 5, 2019

amitkrout Sep 6, 2019

amitkrout Sep 6, 2019

amitkrout commented Sep 6, 2019

amitkrout left a comment

cdrage commented Sep 6, 2019

amitkrout Sep 6, 2019

amitkrout commented Sep 6, 2019

amitkrout left a comment

openshift-ci-robot commented Sep 9, 2019

girishramnani commented Sep 9, 2019

openshift-bot commented Sep 9, 2019

Adds odo catalog list services -o json #2066

Adds odo catalog list services -o json #2066

Conversation

cdrage commented Aug 30, 2019 • edited

cdrage commented Aug 30, 2019 • edited

cdrage commented Sep 3, 2019

dharmit left a comment

Choose a reason for hiding this comment

dharmit Sep 4, 2019

Choose a reason for hiding this comment

cdrage Sep 5, 2019

Choose a reason for hiding this comment

dharmit Sep 9, 2019

Choose a reason for hiding this comment

amitkrout left a comment • edited

Choose a reason for hiding this comment

cdrage commented Sep 5, 2019

amitkrout Sep 6, 2019

Choose a reason for hiding this comment

amitkrout Sep 6, 2019

Choose a reason for hiding this comment

amitkrout commented Sep 6, 2019

amitkrout left a comment

Choose a reason for hiding this comment

cdrage commented Sep 6, 2019

amitkrout Sep 6, 2019

Choose a reason for hiding this comment

amitkrout commented Sep 6, 2019

amitkrout left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 9, 2019

girishramnani commented Sep 9, 2019

openshift-bot commented Sep 9, 2019

Adds `odo catalog list services -o json` #2066

Adds `odo catalog list services -o json` #2066

cdrage commented Aug 30, 2019 •

edited

cdrage commented Aug 30, 2019 •

edited

amitkrout left a comment •

edited