Refactor catalog code #2133

cdrage · 2019-09-16T19:23:12Z

This PR:

Changes how we use odo catalog list services (using ServiceTypeList
and ServiceType)
Changes how we use `odo catalog list components (using
ComponentTypeList and ComponentType)
Refactors the code from occlient.go and puts it inside of
catalog/catalog.go
Changes how to we output services by using ServiceTypeList
Changes the API structure of odo catalog list services

To test:

odo catalog list services
odo catalog list components

See below for an example of the updated json:

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

pkg/catalog/catalog.go

pkg/catalog/catalog_test.go

dharmit

Mostly comments. Not sure if they would end up having to make changes to the code.

dharmit · 2019-09-18T11:21:41Z

pkg/catalog/catalog.go

+
+	clusterServiceClasses, err := getClusterCatalogServices(client)
+	if err != nil {
+		return ServiceTypeList{}, errors.Wrapf(err, "unable to get cluster serviceClassExternalName")


Is there some typo in this error message?

Also, the error message (in its current form) seems likely to confuse the CLI user instead of help them understand what failed.

No typo, this is from previous code.

dharmit · 2019-09-18T11:25:48Z

pkg/catalog/catalog.go

+
+	classes, err := client.GetClusterServiceClasses()
+	if err != nil {
+		return nil, errors.Wrap(err, "unable to get cluster service classes")


Is this error message going to show up when odo catalog list services fails? I'm kind of confused as to what a cluster service class means and am wondering if the users might end up in similar situation as well.

Nope this is normal. This was from previous code.

pkg/catalog/types.go

dharmit · 2019-09-18T11:29:51Z

pkg/catalog/types.go

+}
+
+// ServiceType ...
+type ServiceType struct {


Same as ComponentType. Why not just Service?

also about the // ServiceType ... would having more info in the comment help?

I discussed with @kadel and ServiceType and Component Type is a better fit than just Service and Component

more info in the comment?

dharmit · 2019-09-18T11:35:13Z

pkg/odo/cli/catalog/list/components.go

-	if len(unsupCatalogList) != 0 {
-		fmt.Fprintln(w, "OpenShift Components:")
-		o.printCatalogList(w, unsupCatalogList)
+		w.Flush()


Is this output expected:

$ ./odo catalog list components OpenShift Components: NAME PROJECT TAGS dotnet openshift 2.0,latest httpd openshift 2.4,latest nginx openshift 1.10,1.12,1.8,latest nodejs openshift 10,6,8,8-RHOAR,latest perl openshift 5.24,5.26,latest php openshift 7.0,7.1,latest python openshift 2.7,3.5,3.6,latest ruby openshift 2.3,2.4,2.5,latest wildfly openshift 10.0,10.1,11.0,12.0,13.0,8.1,9.0,latest

I was expecting list of supported & unsupported catalog components.

this is because of #2129. you can use the PSI cluster which should show you supported and unsupported

Why do we need to check aginst PSI cluster? If I check against any other cluster, it won't show what's supported and what's not? 🤔

upstream images vs downstream images. and we have PR which will resolve it - #2129

upstream images have different repositories and do observe that the above list doesn't even mention java, this is because due to licensing upstream okd cannot have java shipped in it.
And as per nodejs images, the repository urls for component image streams are different between upstream and downstream openshift.
We added the downstream images in supported list. but forgot to add the upstream images.

also this is not in scope of this PR and hence can be done on a separate issue related to "improving catalog component output"
@dharmit WDYT?

removing the hold

Unsupported is an ambiguous term which could be taken by the user as that they cannot even use odo with their cluster.
The current output in my opinion effective and actually adapts to this scenario well.

How we can say that on one cluster we officially support the image, and on the other one we don't?

also this is not in scope of this PR and hence can be done on a separate issue related to "improving catalog component output"
@dharmit WDYT?

Yes, but this is still an issue that needs to be fixed. @dharmit can you open a new issue for this?
This is a bug that needs to be fixed. Good job noticing it @dharmit.

pkg/catalog/types.go

girishramnani · 2019-09-19T11:40:04Z

are all the comments addressed @cdrage?

cdrage · 2019-09-19T12:04:32Z

@girishramnani yes, I marked them all as resolved.

Please do another review @girishramnani @dharmit @amitkrout

girishramnani · 2019-09-19T12:34:59Z

pkg/catalog/types.go

+}
+
+// ServiceSpec is the spec for ServiceType
+type ServiceSpec struct {


nit: ServiceTypeSpec then?

ServiceSpec seems okay for now as discussed with @kadel

fine by me then

dharmit · 2019-09-19T12:35:05Z

/hold

Till we reach a conclusion on #2133 (comment). I'm fine being wrong about it but just want to make sure that something's not wrong.

amitkrout

I can see some minimal changes in the test files which looks good me, however i am not adding any label to it because i am not confident on rest of the code changes.

I would expect review from reviewer in the list

girishramnani · 2019-09-19T12:43:26Z

/hold cancel

cdrage · 2019-09-19T12:47:39Z

#2133 (comment) should be fixed outside of this PR as it's unrelated to the JSON support in this PR, correct? It seems like a bug fix that needs to be fixed separately.

girishramnani · 2019-09-19T12:50:47Z

yes this fix should be outside of the PR

dharmit · 2019-09-19T12:55:50Z

#2133 (comment) should be fixed outside of this PR as it's unrelated to the JSON support in this PR, correct? It seems like a bug fix that needs to be fixed separately.

I agree that this should be fixed outside this PR. The "hold" was only meant to reach a mutual conclusion on the topic and understand if the absence of "Unsupported" image is a problem or not.

girishramnani · 2019-09-19T13:05:29Z

/approve

openshift-ci-robot · 2019-09-19T13:05:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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~~ [girishramnani]

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

kadel · 2019-09-19T13:06:20Z

/hold

I had a lot of comments, and want to have one more look before this gets merged.

kadel

In a few places, you forgot to fill information for APIVersion.

No other blockers.

The above issue made me think. Wouldn't it be more convenient to have functions that instantiate the structs? For example, NewComponentType that would return an instance of a new ComponentType with all MetaType fields filled. Then you would be able to fill those values just once in one commonplace.

It is up to you whatever you want to do this or not. I think that it would make it more maintainable. But it is up to you @cdrage

pkg/catalog/catalog.go

cdrage · 2019-09-19T15:38:24Z

Thanks @kadel

For now I'll put off with adding a new function until I start implementing other components of json. Not saying I'm lazy, but I actually quite like how we view it as:

  return ComponentTypeList{
    TypeMeta: metav1.TypeMeta{
      Kind:       "ComponentTypeList",
      APIVersion: "odo.openshift.io/v1alpha1",
    },  
    Items: catalogList,
  }, nil

I'd like to however find a way to create a universal output with a standard TypeMeta set however.

cdrage · 2019-09-19T17:12:48Z

/test v4.2-e2e-scenarios

This PR: - Changes how we use `odo catalog list services` (using ServiceTypeList and ServiceType) - Changes how we use `odo catalog list components (using ComponentTypeList and ComponentType) - Refactors the code from occlient.go and puts it inside of catalog/catalog.go - Changes how to we output services by using `ServiceTypeList` - Changes the API structure of `odo catalog list services` To test: ```sh odo catalog list services odo catalog list components ``` See below for an example of the updated json: ```json { "kind": "ServiceTypeList", "apiVersion": "odo.openshift.io/v1alpha1", "metadata": { "creationTimestamp": null }, "items": [ { "kind": "ServiceType", "metadata": { "name": "cakephp-mysql-persistent", "creationTimestamp": null }, "spec": { "hidden": false, "planList": [ "default" ] } }, { "kind": "ServiceType", "metadata": { "name": "dancer-mysql-persistent", "creationTimestamp": null }, "spec": { "hidden": false, "planList": [ "default" ] } }, } } ```

kadel · 2019-09-20T05:13:13Z

/retest

level=warning msg="Found override for ReleaseImage. Please be warned, this is not advised"
level=info msg="Consuming \"Install Config\" from target directory"
level=info msg="Creating infrastructure resources..."
level=error
level=error msg="Error: Error applying plan:"
level=error
level=error msg="1 error occurred:"
level=error msg="\t* module.vpc.aws_lb.api_external: 1 error occurred:"
level=error msg="\t* aws_lb.api_external: timeout while waiting for state to become 'active' (last state: 'provisioning', timeout: 20m0s)"
level=error
level=error
level=error

kadel · 2019-09-20T06:03:36Z

/hold cancel
/lgtm

dharmit · 2019-09-20T06:54:14Z

@amitkrout I don't understand what could be the reason of failure here:

2019/09/20 05:13:35 Resolved source https://github.com/openshift/odo to master@a0a0a5f2, merging: #2133 fcff75fd @cdrage
2019/09/20 05:13:35 Using namespace ci-op-8l20dt6q
2019/09/20 05:13:35 Running root, [release-inputs], src, [images], [release:latest], e2e-scenarios
2019/09/20 05:13:35 Creating namespace ci-op-8l20dt6q
2019/09/20 05:13:35 Creating rolebinding for user cdrage in namespace ci-op-8l20dt6q
2019/09/20 05:13:35 Setting a soft TTL of 1h0m0s for the namespace
2019/09/20 05:13:35 Setting a hard TTL of 12h0m0s for the namespace
2019/09/20 05:13:35 Setting up pipeline imagestream for the test
2019/09/20 05:13:35 Created secret e2e-scenarios-cluster-profile
2019/09/20 05:13:35 Building root
2019/09/20 05:13:35 Tagged shared images from ocp/4.2:${component}, images will be pullable from registry.svc.ci.openshift.org/ci-op-8l20dt6q/stable:${component}
2019/09/20 05:13:43 Create release image registry.svc.ci.openshift.org/ci-op-8l20dt6q/release:latest
2019/09/20 05:14:18 Copied 2.11Mi of artifacts from release-latest to /logs/artifacts/release-latest
2019/09/20 05:15:51 Build root succeeded after 2m19s
2019/09/20 05:15:51 Building src
2019/09/20 05:17:04 Build src succeeded after 1m12s
2019/09/20 05:17:04 Executing template e2e-scenarios
2019/09/20 05:17:04 Creating or restarting template instance
2019/09/20 05:17:04 Waiting for template instance to be ready
2019/09/20 05:17:06 Running pod e2e-scenarios
2019/09/20 05:17:09 Container cli in pod e2e-scenarios completed successfully
2019/09/20 06:00:02 Container setup in pod e2e-scenarios completed successfully
2019/09/20 06:15:18 Container test in pod e2e-scenarios completed successfully
2019/09/20 06:15:30 Container lease in pod e2e-scenarios completed successfully
2019/09/20 06:23:07 Container teardown in pod e2e-scenarios completed successfully
2019/09/20 06:23:07 Pod e2e-scenarios succeeded after 1h6m2s
2019/09/20 06:23:14 Copied 114.06Mi of artifacts from e2e-scenarios to /logs/artifacts/e2e-scenarios
2019/09/20 06:23:15 Ran for 1h9m40s

/test v4.2-e2e-scenarios

openshift-ci-robot added the size/XL label Sep 16, 2019

openshift-ci-robot requested review from amitkrout and dharmit September 16, 2019 19:23

girishramnani self-requested a review September 17, 2019 08:50

girishramnani reviewed Sep 17, 2019

View reviewed changes

pkg/catalog/catalog.go Outdated Show resolved Hide resolved

girishramnani reviewed Sep 17, 2019

View reviewed changes

pkg/catalog/catalog.go Show resolved Hide resolved

girishramnani reviewed Sep 17, 2019

View reviewed changes

pkg/catalog/catalog_test.go Show resolved Hide resolved

girishramnani reviewed Sep 17, 2019

View reviewed changes

pkg/catalog/catalog_test.go Show resolved Hide resolved

cdrage force-pushed the refactor-catalog-list-services branch from 1be1dcb to 86f12f0 Compare September 17, 2019 13:58

dharmit suggested changes Sep 18, 2019

View reviewed changes

kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 18, 2019

kadel reviewed Sep 18, 2019

View reviewed changes

pkg/catalog/types.go Outdated Show resolved Hide resolved

cdrage force-pushed the refactor-catalog-list-services branch from 86f12f0 to 97b1157 Compare September 18, 2019 13:22

cdrage changed the title ~~Refactor odo catalog list services / catalog code~~ Refactor catalog code Sep 18, 2019

cdrage force-pushed the refactor-catalog-list-services branch from 97b1157 to a292ad6 Compare September 18, 2019 13:36

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 18, 2019

cdrage mentioned this pull request Sep 18, 2019

Add odo catalog list components -o json #2067

Closed

cdrage force-pushed the refactor-catalog-list-services branch 2 times, most recently from acf1c98 to abd0c22 Compare September 18, 2019 18:19

cdrage force-pushed the refactor-catalog-list-services branch from abd0c22 to e2eb5b0 Compare September 19, 2019 12:34

girishramnani reviewed Sep 19, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 19, 2019

amitkrout reviewed Sep 19, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 19, 2019

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 19, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 19, 2019

dharmit mentioned this pull request Sep 19, 2019

Should we mention "Supported" / "Unsupported" in components list output for community images? #2148

Closed

kadel requested changes Sep 19, 2019

View reviewed changes

pkg/catalog/catalog.go Outdated Show resolved Hide resolved

pkg/catalog/catalog.go Outdated Show resolved Hide resolved

cdrage force-pushed the refactor-catalog-list-services branch from e2eb5b0 to ed8a5f6 Compare September 19, 2019 15:34

cdrage force-pushed the refactor-catalog-list-services branch from ed8a5f6 to 34a8384 Compare September 19, 2019 15:44

cdrage force-pushed the refactor-catalog-list-services branch from 34a8384 to fcff75f Compare September 19, 2019 19:13

cdrage mentioned this pull request Sep 19, 2019

Update machine output for project delete / create #2150

Closed

kadel approved these changes Sep 20, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 20, 2019

openshift-ci-robot assigned kadel Sep 20, 2019

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

openshift-merge-robot merged commit a6442a8 into redhat-developer:master Sep 20, 2019

dharmit mentioned this pull request Sep 26, 2019

Bump odo version to 1.0.0-beta6 #2182

Merged

jichenjc mentioned this pull request Mar 20, 2020

'odo catalog list components -o json' does not have imageStreamRef in json output after update from v1.0.x to v1.1.0 #2707

Closed

cdrage deleted the refactor-catalog-list-services branch January 14, 2022 14:54

rm3l added the estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person label Jun 19, 2023

Refactor catalog code #2133

Refactor catalog code #2133

Conversation

cdrage commented Sep 16, 2019 • edited

dharmit 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Sep 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani commented Sep 19, 2019

cdrage commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharmit commented Sep 19, 2019

amitkrout left a comment

Choose a reason for hiding this comment

girishramnani commented Sep 19, 2019

cdrage commented Sep 19, 2019

girishramnani commented Sep 19, 2019

dharmit commented Sep 19, 2019

girishramnani commented Sep 19, 2019

openshift-ci-robot commented Sep 19, 2019

kadel commented Sep 19, 2019

kadel left a comment

Choose a reason for hiding this comment

cdrage commented Sep 19, 2019

cdrage commented Sep 19, 2019

kadel commented Sep 20, 2019

kadel commented Sep 20, 2019

dharmit commented Sep 20, 2019

cdrage commented Sep 16, 2019 •

edited

kadel Sep 19, 2019 •

edited