list devfile components catalog in json format

[yangcao77]

Signed-off-by: Stephanie [email protected]

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug
/area devfile

What does does this PR do / why we need it:

This PR fixes the bug that run catalog list component in json format does not print devfile catalog components.

Which issue(s) this PR fixes:

Fixes #3247

How to test changes / Special notes to the reviewer:
create a devfile component and run odo catalog list component -o json

{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "s2iItems": [
        {
            "kind": "ComponentType",
            "apiVersion": "odo.dev/v1alpha1",
            "metadata": {
                "name": "dotnet",
                "namespace": "openshift",
                "creationTimestamp": null
            },
            "spec": {
                "allTags": [
                    "2.0",
                    "latest"
                ],
                "nonHiddenTags": [
                    "2.0",
                    "latest"
                ],
                "supportedTags": []
            }
        },
    ]
    "devfileItems": [
        {
            "Name": "openLiberty",
            "DisplayName": "Open Liberty",
            "Description": "Open Liberty microservice in Java",
            "Link": "/devfiles/openLiberty/devfile.yaml",
            "Support": true,
            "Registry": {
                "Name": "DefaultDevfileRegistry",
                "URL": "https://raw.githubusercontent.com/elsony/devfile-registry/master"
            }
        },
    ]
}

(Just to show the format, not the actual result. The actual result contains more items. )

[dharmit]

@yangcao77 the PR is doing what's mentioned in the "How to test changes" section. However, IMO, printing the list of registries should be avoided in the output of odo catalog list components -o json. It's more appropriate for odo registry list -o json, no?

Besides that, if we look at the output of the command for s2i based components, it is:

{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
        {
            "kind": "ComponentType",
            "apiVersion": "odo.dev/v1alpha1",
            "metadata": {
                "name": "dotnet",
                "namespace": "openshift",
                "creationTimestamp": null
            },
            "spec": {
                "allTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "nonHiddenTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "supportedTags": []
            }
        },
    ]                                                                                                                                                                                         
}

I'm thinking from the plugin developer point of view. Should we try to adhere to the output format above? Specifically the part:

 "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
     devfile components' json here.
    ]

IMO, this will be a breaking change for them otherwise. @kadel @elsony WDYT?

[yangcao77]

There are two lists defined: catalogList and catalogDevfileList, for odo component and devfile registry respectively. And the item types are different as well.
Ping @GeekArthur since he implemented it.
Any reason we did not combined catalogList and catalogDevfileList? (i.e. componentTypeandDevfileComponentType`)

[GeekArthur]

Any reason we did not combined catalogList and catalogDevfileList? (i.e. componentType and DevfileComponentType`)

componentType and DevfileComponentType are totally different structs and they have totally different fields inside, that's why we can't combine.

However, IMO, printing the list of registries should be avoided in the output of odo catalog list components -o json. It's more appropriate for odo registry list -o json, no?

List registry information for each DEVFILE component is necessary because we already have dynamic registry support and we might hit the case where same component name exist in different registry. In order to resolve the naming conflict issue, we need to record which component belongs to which registry. Same for odo create, we use --registry to specify the registry when creating component.

That being said, I find this PR directly calls machineoutput.OutputSuccess(o.catalogDevfileList) to output the json format, instead of doing that I think we should:

1. Have a design of expected json output content/format to ensure the upstream project such as IDE plugin can consume the json output like @dharmit points out so that avoid breaking change.
2. Modify the existing json converter/code base according to the design.

[yangcao77]

@dharmit
Discussed with @elsony @GeekArthur , running odo catalog list components is also separating out the output into two list.

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

Odo Devfile Components:
NAME                 DESCRIPTION                            REGISTRY                   SUPPORTED
openLiberty          Open Liberty microservice in Java      DefaultDevfileRegistry     YES
java-spring-boot     Spring Boot® using Java                DefaultDevfileRegistry     YES
quarkus              Upstream Quarkus with Java+GraalVM     DefaultDevfileRegistry     YES
nodejs               Stack with NodeJS 10                   DefaultDevfileRegistry     YES
maven                Upstream Maven and OpenJDK 11          DefaultDevfileRegistry     YES

Considering -o json is only presenting the output in json format, we should keep to use two separate lists.

In addition, when IDE plugin consumes the json output, they need to take care of devfile registries specifically anyways since the infomation need to be displayed is different from s2i components.

[dharmit]

List registry information for each DEVFILE component is necessary because we already have dynamic registry support and we might hit the case where same component name exist in different registry. In order to resolve the naming conflict issue, we need to record which component belongs to which registry.

@GeekArthur I think you're referring to the registry information printed in individual components:

"Items": [
   {
       "Name": "java-gradle",
       "DisplayName": "Java Gradle",
       "Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
       "Link": "/devfiles/java-gradle/devfile.yaml",
       "Support": false,
       "Registry": {
           "Name": "CheDevfileRegistry",
           "URL": "https://che-devfile-registry.openshift.io"
       }
   }
]

I agree that having registry information in above JSON data makes sense for exactly the reasons you mentioned. However, I'm referring to following data that's showing up in odo catalog list components -o json:

"DevfileRegistries": {
        "CheDevfileRegistry": {
            "Name": "CheDevfileRegistry",
            "URL": "https://che-devfile-registry.openshift.io"
        },
        "DefaultDevfileRegistry": {
            "Name": "DefaultDevfileRegistry",
            "URL": "https://raw.githubusercontent.com/elsony/devfile-registry/master"
        }
    }
]

IMO, above information is something that's more suited for odo registry list -o json and is a bit redundant as well since we're already printing the registry info for each component in odo catalog list components -o json. WDYT?

[dharmit]

@yangcao77

There are two lists defined: catalogList and catalogDevfileList, for odo component and devfile registry respectively. And the item types are different as well.
Considering -o json is only presenting the output in json format, we should keep to use two separate lists.

@GeekArthur

I think we should:

1. Have a design of expected json output content/format to ensure the upstream project such as IDE plugin can consume the json output like @dharmit points out so that avoid breaking change.

2. Modify the existing json converter/code base according to the design.

Sorry for the communication gap from my side. I agree with having two separate lists that represent s2i & devfile components. However, I was suggesting that in the list for devfile components, we adhere to the format used in s2i components. I'll try to explain it through below codeblocks:

s2i json

{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
        {
            "kind": "ComponentType",
            "apiVersion": "odo.dev/v1alpha1",
            "metadata": {
                "name": "dotnet",
                "namespace": "openshift",
                "creationTimestamp": null
            },
            "spec": {
                "allTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "nonHiddenTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "supportedTags": []
            }
        },
    ]                                                                                                                                                                                         
}

Current json for devfile components (as per PR description)

{
    "DevfileRegistries": {
        "CheDevfileRegistry": {
            "Name": "CheDevfileRegistry",
            "URL": "https://che-devfile-registry.openshift.io"
        },
        "DefaultDevfileRegistry": {
            "Name": "DefaultDevfileRegistry",
            "URL": "https://raw.githubusercontent.com/elsony/devfile-registry/master"
        }
    },
    "Items": [
        {
            "Name": "java-gradle",
            "DisplayName": "Java Gradle",
            "Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
            "Link": "/devfiles/java-gradle/devfile.yaml",
            "Support": false,
            "Registry": {
                "Name": "CheDevfileRegistry",
                "URL": "https://che-devfile-registry.openshift.io"
            }
        }
    ]
}

Changes I am proposing for devfile json output

{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
        {
            "Name": "java-gradle",
            "DisplayName": "Java Gradle",
            "Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
            "Link": "/devfiles/java-gradle/devfile.yaml",
            "Support": false,
            "Registry": {
                "Name": "CheDevfileRegistry",
                "URL": "https://che-devfile-registry.openshift.io"
            }
        },
    ]
}

How they would look combined?

{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
        {
            "kind": "ComponentType",
            "apiVersion": "odo.dev/v1alpha1",
            "metadata": {
                "name": "dotnet",
                "namespace": "openshift",
                "creationTimestamp": null
            },
            "spec": {
                "allTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "nonHiddenTags": [
                    "2.1",
                    "3.0",
                    "3.1",
                    "latest"
                ],
                "supportedTags": []
            }
        },
    ]
}
{
    "kind": "List",
    "apiVersion": "odo.dev/v1alpha1",
    "metadata": {
        "creationTimestamp": null
    },
    "items": [
           {
            "Name": "java-gradle",
            "DisplayName": "Java Gradle",
            "Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
            "Link": "/devfiles/java-gradle/devfile.yaml",
            "Support": false,
            "Registry": {
                "Name": "CheDevfileRegistry",
                "URL": "https://che-devfile-registry.openshift.io"
            }
        }
    ]
}

---

This still makes me wonder if the JSON output would be usable by the IDE plugins? The reason I'm thinking that way is because we'll be returning two distinct lists here. Ideally, we could list s2i components in one list and devfile components in another, but keep them in single JSON output. Something like:

Combined lists

{
	"kind": "List",
	"apiVersion": "odo.dev/v1alpha1",
	"metadata": {
		"creationTimestamp": null
	},
	"s2iItems": [
		[{
			"kind": "ComponentType",
			"apiVersion": "odo.dev/v1alpha1",
			"metadata": {
				"name": "dotnet",
				"namespace": "openshift",
				"creationTimestamp": null
			},
			"spec": {
				"allTags": [
					"2.1",
					"3.0",
					"3.1",
					"latest"
				],
				"nonHiddenTags": [
					"2.1",
					"3.0",
					"3.1",
					"latest"
				],
				"supportedTags": []
			}
		}]
	],
	"devfileItems": [
		[{
			"Name": "java-gradle",
			"DisplayName": "Java Gradle",
			"Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
			"Link": "/devfiles/java-gradle/devfile.yaml",
			"Support": false,
			"Registry": {
				"Name": "CheDevfileRegistry",
				"URL": "https://che-devfile-registry.openshift.io"
			}
		}]
	]
}

@yangcao77 @GeekArthur @kadel @elsony need your inputs on this because I'm not sure if I'm thinking along the right lines. To me, it looks like returning two distinct lists will accomplish odo catalog list components -o json from our side but from user (mainly plugin devs) point of view, this won't be very helpful (unless I'm fundamentally wrong about consuming json).

Correct me if I'm wrong.

[kadel]

Two lists are ok, but what odo prints to stdout needs to be valid output.
@dharmit your first example of the combined output is not a valid JSON, but the second one with s2iItems and devfileItems is valid this one could be used.

[dharmit]

@dharmit your first example of the combined output is not a valid JSON, but the second one with s2iItems and devfileItems is valid this one could be used.

Yes, that's exactly my concern. The first output is based on what's proposed in the PR. And I wasn't 100% sure if it's really invalid. Hence the long dump that I've made above.

[GeekArthur]

List registry information for each DEVFILE component is necessary because we already have dynamic registry support and we might hit the case where same component name exist in different registry. In order to resolve the naming conflict issue, we need to record which component belongs to which registry.

@GeekArthur I think you're referring to the registry information printed in individual components:

"Items": [
   {
       "Name": "java-gradle",
       "DisplayName": "Java Gradle",
       "Description": "Java Stack with OpenJDK 11 and Gradle 6.2.1",
       "Link": "/devfiles/java-gradle/devfile.yaml",
       "Support": false,
       "Registry": {
           "Name": "CheDevfileRegistry",
           "URL": "https://che-devfile-registry.openshift.io"
       }
   }
]

I agree that having registry information in above JSON data makes sense for exactly the reasons you mentioned. However, I'm referring to following data that's showing up in odo catalog list components -o json:

"DevfileRegistries": {
        "CheDevfileRegistry": {
            "Name": "CheDevfileRegistry",
            "URL": "https://che-devfile-registry.openshift.io"
        },
        "DefaultDevfileRegistry": {
            "Name": "DefaultDevfileRegistry",
            "URL": "https://raw.githubusercontent.com/elsony/devfile-registry/master"
        }
    }
]

IMO, above information is something that's more suited for odo registry list -o json and is a bit redundant as well since we're already printing the registry info for each component in odo catalog list components -o json. WDYT?

Yes, only registry information for each individual devfile component is must-have, we can filter out the global registry information in json output format. Also the combined list that your propose looks good to me, we can use that as a design format to implement.

[yangcao77]

@dharmit Pushed a new commit, the output is now like what you purposed. Can you help review again? Thanks

[dharmit]

@yangcao77 I think the resulting JSON is still invalid. I've pulled the change you made. In odo catalog list components -o json we're returning two distinct and valid JSON lists which when combined become invalid.

I'm not a JSON expert. I used https://jsonlint.com/ for validating.

If you look at the "Combined Lists" in my earlier comment, it's a valid JSON (I made some minor change to it right now because, well, I'm not JSON expert 😄 ). I've created two keys there - s2iItems and devfileItems - as suggestions. We can discuss if you or anyone has any other suggestions.

[yangcao77]

@yangcao77 I think the resulting JSON is still invalid. I've pulled the change you made. In odo catalog list components -o json we're returning two distinct and valid JSON lists which when combined become invalid.

I'm not a JSON expert. I used https://jsonlint.com/ for validating.

If you look at the "Combined Lists" in my earlier comment, it's a valid JSON (I made some minor change to it right now because, well, I'm not JSON expert 😄 ). I've created two keys there - s2iItems and devfileItems - as suggestions. We can discuss if you or anyone has any other suggestions.

Oops. I only saw you first example, didn't notice there is a "Combined Lists" at the bottom.
By doing this, it's not two separated lists, it's a combined list from the JSON perspective.
I've pushed a new commit to make that change.

[dharmit]

@yangcao77 Thank you for making the changes.

@GeekArthur @kadel @yangcao77 Thanks for bearing with me while I dump my thoughts in looooong comments. 😆

/approve

NOTE: We'll have to cover this PR in changelog for next release since it would break things for IDE plugins if Experimental mode is enabled.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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:

• pkg/catalog/OWNERS [dharmit]
• pkg/odo/cli/catalog/OWNERS [dharmit]
• tests/OWNERS [dharmit]

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

[yangcao77]

@dharmit Can we get this approve now as we are in sprint 185? Or we need to wait till 186?

[dharmit]

@dharmit Can we get this approve now as we are in sprint 185? Or we need to wait till 186?

We need someone to review the PR. Doesn't have anything to do with the sprint. What I have done is checked the functionality of the PR and we made changes to certain functionality. But someone needs to check the code and do an lgtm.

[GeekArthur]

We can remove this temporary debug comment.

[yangcao77]

removed.

[GeekArthur]

Overall it looks good, add couple of comments below:

1. We should handle the case where there is no devfile registry at least for non-experimental mode. For example, if I set Experimental to false and run odo catalog list components -o json I can see "devfileItems": null from the json output, we shouldn't display any devfile related information on non-experimental mode. I think the ideal way to fix this is that once we detect o.catalogDevfileList.Items is empty then we don't display that field to user in json format, with that fix we don't need to handle the case for non-experimental explicitly and it also benefits experimental mode because we don't display it instead of displaying "devfileItems": null
2. Currently the PR description is This PR fixes the bug that run catalog list component in json format does not print devfile registries., this PR is for listing devfile components in json format, it's not related with devfile registries, we should update that. Also this PR is not for fixing bug, it's for adding feature. Reason to point out this is to avoiding misleading Charlie when he is working on odo feature release.

[yangcao77]

Overall it looks good, add couple of comments below:

1. We should handle the case where there is no devfile registry at least for non-experimental mode. For example, if I set Experimental to false and run `odo catalog list components -o json` I can see `"devfileItems": null` from the json output, we shouldn't display any devfile related information on non-experimental mode. I think the ideal way to fix this is that once we detect `o.catalogDevfileList.Items` is empty then we don't display that field to user in json format, with that fix we don't need to handle the case for non-experimental explicitly and it also benefits experimental mode because we don't display it instead of displaying `"devfileItems": null`

2. Currently the PR description is `This PR fixes the bug that run catalog list component in json format does not print devfile registries.`, this PR is for listing devfile components in json format, it's not related with devfile registries, we should update that. Also this PR is not for fixing bug, it's for adding feature. Reason to point out this is to avoiding misleading Charlie when he is working on odo feature release.

Good catch. I have updated this PR to not display the s2iItems and devfileItems if the value is null by adding omitempty to the struct definition. Also modifies the integration test to test that.

I tagged the PR as a bug due to the original issue #3247 was tagged as a bug. But I totally agree that we need to bring up attention to all folks because the json output format has been changed. I will add the topic to contributor call's agenda.

[GeekArthur]

LGTM!

[GeekArthur]

Overall it looks good, add couple of comments below:

1. We should handle the case where there is no devfile registry at least for non-experimental mode. For example, if I set Experimental to false and run `odo catalog list components -o json` I can see `"devfileItems": null` from the json output, we shouldn't display any devfile related information on non-experimental mode. I think the ideal way to fix this is that once we detect `o.catalogDevfileList.Items` is empty then we don't display that field to user in json format, with that fix we don't need to handle the case for non-experimental explicitly and it also benefits experimental mode because we don't display it instead of displaying `"devfileItems": null`

2. Currently the PR description is `This PR fixes the bug that run catalog list component in json format does not print devfile registries.`, this PR is for listing devfile components in json format, it's not related with devfile registries, we should update that. Also this PR is not for fixing bug, it's for adding feature. Reason to point out this is to avoiding misleading Charlie when he is working on odo feature release.

Good catch. I have updated this PR to not display the s2iItems and devfileItems if the value is null by adding omitempty to the struct definition. Also modifies the integration test to test that.

I tagged the PR as a bug due to the original issue #3247 was tagged as a bug. But I totally agree that we need to bring up attention to all folks because the json output format has been changed. I will add the topic to contributor call's agenda.

Thanks for making the changes, your approach is better! The PR looks good to me.

[openshift-bot]

/retest

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

[codecov]

Codecov Report

Merging #3264 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3264      +/-   ##
==========================================
+ Coverage   45.54%   45.57%   +0.02%     
==========================================
  Files         111      111              
  Lines       10972    10972              
==========================================
+ Hits         4997     5000       +3     
+ Misses       5486     5483       -3     
  Partials      489      489              

Impacted Files	Coverage Δ
pkg/catalog/catalog.go	50.44% <ø> (ø)
pkg/sync/sync.go	48.51% <0.00%> (+2.97%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da08b6...0470ac7. Read the comment docs.

[openshift-bot]

/retest

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