List components from other sources #4742

valaparthvi · 2021-05-24T07:45:10Z

What type of PR is this?
/kind feature
/kind code-refactoring

What does this PR do / why we need it:
This PR adds the support to see components created/managed by tools other than odo.

Which issue(s) this PR fixes:

Fixes #4688

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Manually create a deployment and deploymentconfig in openshift web console or any other tool. Make sure it is labelled with the following labels -

        app: httpd
        app.kubernetes.io/instance: example
        app.kubernetes.io/managed-by: console
        app.kubernetes.io/managed-by-version: v4.8.0
        app.kubernetes.io/name: example
        app.kubernetes.io/part-of: httpd

Or better yet, it follows these labels - https://github.com/redhat-developer/app-labels

With OCP4, it will be easy to create a component of a deployment kind by using the samples provided by ocp, but to create a deploymentConfig kind component you can use the following yaml -

apiVersion: apps.openshift.io/v1
kind: DeploymentConfig
metadata:
  name: example
  namespace: myproject
  labels:
    app: httpd
    app.kubernetes.io/instance: example
    app.kubernetes.io/managed-by: console
    app.kubernetes.io/managed-by-version: v4.8.0
    app.kubernetes.io/name: example
    app.kubernetes.io/part-of: httpd
spec:
  selector:
    app: httpd
  replicas: 3
  template:
    metadata:
      labels:
        app: httpd
        app.kubernetes.io/instance: example
        app.kubernetes.io/managed-by: console
        app.kubernetes.io/managed-by-version: v4.8.0
        app.kubernetes.io/name: example
        app.kubernetes.io/part-of: httpd
    spec:
      containers:
        - name: httpd
          image: >-
            image-registry.openshift-image-registry.svc:5000/openshift/httpd:latest
          ports:
            - containerPort: 8080

Also create a few components with the help of odo.

Run odo list and you should be able to see the non-odo components in the output that looks something like this -

➜  odo git:(listcom) ✗ odo list --all-apps
W0524 13:12:51.685576   13526 warnings.go:70] extensions/v1beta1 Ingress is deprecated in v1.14+, unavailable in v1.22+; use networking.k8s.io/v1 Ingress
W0524 13:12:55.884893   13526 warnings.go:70] extensions/v1beta1 Ingress is deprecated in v1.14+, unavailable in v1.22+; use networking.k8s.io/v1 Ingress
W0524 13:12:58.278964   13526 warnings.go:70] extensions/v1beta1 Ingress is deprecated in v1.14+, unavailable in v1.22+; use networking.k8s.io/v1 Ingress
Devfile Components: 
APP      NAME      PROJECT       TYPE      STATE
app1     njs2i     myproject     njs2i     Pushed

Other Components running on the cluster(read-only): 
APP            NAME              PROJECT       TYPE
sample-app     nodejs-sample     myproject     nodejs-sample
httpd          example           myproject     example

Also, do test with odo list --app sample-app, odo list --all-apps -o json, odo list.

Note - This is work-in-progress and you might find some commented code and incomplete tests, I am yet to add unit and integrations tests as well, but I am waiting on it to make sure that everyone agrees on the implmentation.

valaparthvi · 2021-05-24T09:11:53Z

pkg/component/component.go

 			if err != nil {
 				return ComponentList{}, errors.Wrap(err, "Unable to get component")
 			}


I have a question here. Why do we return if err!=nil even though there are some other checks pending in the function, for e.g. this check - https://github.com/openshift/odo/pull/4742/files#diff-a84453ec52dba47b60234fd3aba56849e52e2bae2577c082dcf1a79ef6b25855R946 ?

if client != nil {

Why do we avoid that and return? Shouldn't we check for if err == nil { continue } and execute rest of the code? I have seen this type of logic in almost all places. For some it makes sense, but for others I think we should continue executing the program and if it does not make sense to do so, then perhaps we should add a comment about why we are returning without executing the rest of the code.

Other examples are -

pkg/odo/cli/component/list.go:264 - https://github.com/openshift/odo/pull/4742/files#diff-a5ee7fb0c76cd592580dce46cda117d8a483452979b0da40d644906f88cba343R264

pkg/odo/cli/component/list.go:274 - https://github.com/openshift/odo/pull/4742/files#diff-a5ee7fb0c76cd592580dce46cda117d8a483452979b0da40d644906f88cba343R274

..and many more

valaparthvi · 2021-05-24T13:36:52Z

tests/integration/component.go

+		Context("when components are not created/managed by odo", func() {
+			var appName, compName string
+			JustBeforeEach(func() {
+				//CreateNonOdoComponent


Can someone help me with this one? I plan on creating a deployment and deployment-config manually via API, but I need to prepare a lot of data before doing that. Do we already have helper functions that might help here?

you can just create deployment.yaml with the most simple deployment that you can come up with. Add the same labels to it as WebConsole does and just call kubectl apply -f deplyomet.yaml

girishramnani · 2021-06-10T10:37:02Z

pkg/component/component.go

@@ -1651,13 +1642,16 @@ func GetMachineReadableFormatForList(s2iComps []Component) ComponentList {
 }

 // GetMachineReadableFormatForCombinedCompList returns list of devfile and s2i components in machine readable format


maybe update this comment?

girishramnani · 2021-06-10T10:42:43Z

pkg/util/util.go

+// To pass operands such as !=, append a ! prefix to the value.
+// For E.g. map[string]string{"app.kubernetes.io/managed-by": "!odo"}
+// Using != operators also means that resource will be filtered even if it doesn't have the key.
+// So a resource not labelled with key "app.kubernetes.io/managed-by" will also be returned.
 func ConvertLabelsToSelector(labels map[string]string) string {


why not extend the labels here to be a list of structs here like

struct { Key string Value string Operand Operand } type Operand string var Equal Operand = "=" var NotEqual Operand = "!="

might be more future proof?

Can you elaborate?

instead of this label map

labels map[string]string

struct { Key string Value string Operand Operand } type Operand string var Equal Operand = "=" var NotEqual Operand = "!="

So the function would look like this ConvertLabelsToSelector(labels []Label).

So I can do something like this

labels := []Label{ { Key: "app.kubernetes.io/managed-by", Operand: NotEqual, Value: "odo" } } ConvertLabelsToSelector(labels)

The conversion of []Label to string would also become simplified

This would allow for room for extension and also less mistakes

It seems to me as if this will require a significant amount of effort and it seems a little out of scope for this PR. Is it alright if I take this in another PR?

Also, can you give me an example of possible mistakes that could happen here?

if you change strings.Replace(v, "!", "", -1) to strings.Replace(v, "!", "", 1) wouldn't it the problem of doing !!odo?

@kadel I am not sure I understand your ~~problem~~ question correctly.

I am good from my end, please address @kadel's question and then remove the hold

Just realized something. ! can't be present in a label or its value.
I think that we are ok with this. front.

But I think that I'm really missing here is a unit test for this function :-( It became much more complicated than it used to be, and it definetly deserves unit test

@kadel Unit tests! That can be easily fixed! I'll get to it.

@girishramnani I looked at how k8s.io/apimachinery does this label selector thing and it is a little complicated, they have special interface and methods for it. See here - https://github.com/openshift/odo/blob/89ac67e425bffe8c400645bca933d687b5ac8b19/vendor/k8s.io/apimachinery/pkg/fields/selector.go#L430

I will rely on Unit tests for now, and hopefully we will not run into corner cases, or catch them if we do.

kadel · 2021-06-10T11:35:25Z

tested, it it looks good
/approve

valaparthvi · 2021-06-11T10:44:02Z

/test v4.7-integration-e2e

The failing test passes locally. Retrying.

girishramnani · 2021-06-11T12:20:07Z

/hold
Just want to make sure that someone doesn't lgtm until all comments are resolved 🙂

valaparthvi · 2021-06-14T06:31:26Z

/retest
Unit test windows failure.

valaparthvi · 2021-06-14T12:18:51Z

/unhold
Addressed concern in #4742 (comment)

valaparthvi · 2021-06-14T13:18:28Z

/override ci/prow/psi-unit-test-mac
/override ci/prow/psi-unit-test-windows

Reason - These are optional checks.

openshift-ci · 2021-06-14T13:19:33Z

@valaparthvi: Overrode contexts on behalf of valaparthvi: ci/prow/psi-unit-test-mac, ci/prow/psi-unit-test-windows

In response to this:

/override ci/prow/psi-unit-test-mac
/override ci/prow/psi-unit-test-windows

Reason - These are optional checks.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

girishramnani · 2021-06-14T14:15:26Z

/approve
/lgtm

openshift-ci · 2021-06-14T14:15:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani, kadel

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,kadel]

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

kadel · 2021-06-14T15:04:51Z

pkg/util/util_test.go

@@ -2684,3 +2684,48 @@ func TestGetGitOriginPath(t *testing.T) {
 		})
 	}
 }
+
+func TestConvertLabelsToSelector(t *testing.T) {


openshift-bot · 2021-06-14T23:13:25Z

/retest

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

openshift-bot · 2021-06-14T23:26:25Z

/retest

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

valaparthvi requested review from kadel, dharmit, girishramnani, feloy and mik-dass May 24, 2021 07:45

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation kind/code-refactoring labels May 24, 2021

openshift-ci bot requested review from mohammedzee1000 and rnapoles-rh May 24, 2021 07:45

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 24, 2021

valaparthvi force-pushed the listcom branch 3 times, most recently from d720aa1 to 8b668b5 Compare May 24, 2021 09:03

valaparthvi commented May 24, 2021

View reviewed changes

valaparthvi mentioned this pull request May 27, 2021

List components created by sources other than odo #4688

Closed

3 tasks

valaparthvi added 4 commits June 7, 2021 13:40

List components from other sources

aff877c

Rename OdoManagedBy and OdoVersion to ManagedBy and ManagerVersion

fc78532

Add tests

69e72b8

Add deployment and deploymentconfig manifests

1f6da17

valaparthvi force-pushed the listcom branch from 58a2817 to 1f6da17 Compare June 7, 2021 09:54

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 7, 2021

valaparthvi added 3 commits June 8, 2021 12:43

Move tests

9b88443

Test case

17943ec

Add cleaner tests

3bd7e55

valaparthvi force-pushed the listcom branch from 6d8b032 to 3bd7e55 Compare June 8, 2021 17:48

valaparthvi added 2 commits June 9, 2021 14:15

Add comments

6a58cba

Cleanup and fix tests

f528f3a

valaparthvi changed the title ~~WIP: List components from other sources~~ List components from other sources Jun 9, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 9, 2021

girishramnani reviewed Jun 10, 2021

View reviewed changes

Add comment

18cbe7a

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 10, 2021

valaparthvi added 2 commits June 10, 2021 19:16

Fix CI failures

4a7c43d

Change error check

5ea98f6

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Jun 11, 2021

Add unit tests for ConvertLabelToSelector

191cb41

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Jun 14, 2021

Fix unit test CI failure

dfb92bf

openshift-ci bot assigned girishramnani Jun 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 14, 2021

kadel reviewed Jun 14, 2021

View reviewed changes

openshift-merge-robot merged commit 7232538 into redhat-developer:main Jun 15, 2021

dharmit mentioned this pull request Jun 15, 2021

Show components created by other tools in odo #4192

Closed

This was referenced Jan 18, 2022

Non odo components are not visible redhat-developer/vscode-openshift-tools#2324

Closed

Non odo components are not visible redhat-developer/intellij-openshift-connector#369

Closed

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List components from other sources #4742

List components from other sources #4742

valaparthvi commented May 24, 2021 •

edited

valaparthvi May 24, 2021

valaparthvi May 24, 2021

kadel May 26, 2021

girishramnani Jun 10, 2021

girishramnani Jun 10, 2021 •

edited

valaparthvi Jun 10, 2021

girishramnani Jun 11, 2021

girishramnani Jun 11, 2021

valaparthvi Jun 11, 2021

kadel Jun 14, 2021

valaparthvi Jun 14, 2021 •

edited

girishramnani Jun 14, 2021

kadel Jun 14, 2021

valaparthvi Jun 14, 2021

kadel commented Jun 10, 2021

valaparthvi commented Jun 11, 2021

girishramnani commented Jun 11, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

girishramnani commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

kadel Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

		@@ -1651,13 +1642,16 @@ func GetMachineReadableFormatForList(s2iComps []Component) ComponentList {
		}

		// GetMachineReadableFormatForCombinedCompList returns list of devfile and s2i components in machine readable format

List components from other sources #4742

List components from other sources #4742

Conversation

valaparthvi commented May 24, 2021 • edited

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

girishramnani Jun 10, 2021 • edited

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

valaparthvi Jun 14, 2021 • edited

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 commented Jun 10, 2021

valaparthvi commented Jun 11, 2021

girishramnani commented Jun 11, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

valaparthvi commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

girishramnani commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

Choose a reason for hiding this comment

openshift-bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

valaparthvi commented May 24, 2021 •

edited

girishramnani Jun 10, 2021 •

edited

valaparthvi Jun 14, 2021 •

edited