List created components for devfile v2 #3505

girishramnani · 2020-07-07T19:25:42Z

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 feature

What does does this PR do / why we need it:
support listing of components for devfilev2. The --path flag and json output would be added as a follow up.

Which issue(s) this PR fixes:

Fixes #2835
Fixes #3050

How to test changes / Special notes to the reviewer:
odo create nodejs
odo push
odo list

Created a follow up issue #3521 for stuff not covered by the issue story #2835

girishramnani · 2020-07-07T20:04:14Z

tests/integration/devfile/cmd_devfile_push_test.go

+
+		})
+
+		It("checks devfile and s2i components together", func() {


this is a fun one

cdrage

Minor changes.

For some reason there's an issue deploying however..

I used odo create nodejs --starter

~/openshift/stuff                                                                                                                                                                                                                                                               ⍉
▶ odo push

Validation
 ✓  Validating the devfile [44773ns]

Initialization
 ✓  Initializing the component [3s]

Creating Docker resources for component nodejs
 ✓  Pulling image registry.access.redhat.com/ubi8/nodejs-12:1-36 [16s]
 ✓  Starting container for registry.access.redhat.com/ubi8/nodejs-12:1-36 [1s]

Syncing to component nodejs
 ✓  Checking files for pushing [678074ns]
 ✓  Syncing files to the component [331ms]

Executing devfile commands for component nodejs
 ✓  Executing install command "npm install" [2s]
 ✓  Executing run command "npm start" [1s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component

~/openshift/stuff                                                                                                                                                                                                                                                                
▶ odo list
Experimental mode is enabled, use at your own risk

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1591280]

goroutine 1 [running]:
github.com/openshift/odo/pkg/kclient.(*Client).ListDeployments(0x0, 0x1c59402, 0x3, 0x10, 0xc000a4b8c0, 0xc000787b00)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/kclient/deployments.go:50 +0xf0
github.com/openshift/odo/pkg/odo/cli/component.(*ListOptions).Run(0xc000050e00, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/cli/component/list.go:169 +0x1578
github.com/openshift/odo/pkg/odo/genericclioptions.GenericRun(0x1f73640, 0xc000050e00, 0xc0000e6a00, 0x2fe0750, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/genericclioptions/runnable.go:31 +0x13c
github.com/openshift/odo/pkg/odo/cli/component.NewCmdList.func1(0xc0000e6a00, 0x2fe0750, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/cli/component/list.go:277 +0x5e
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).execute(0xc0000e6a00, 0x2fe0750, 0x0, 0x0, 0xc0000e6a00, 0x2fe0750)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:830 +0x2aa
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc00045e500, 0x1d6cc10, 0xc000529860, 0x1)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:914 +0x2fb
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).Execute(...)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:864
main.main()
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/cmd/odo/odo.go:67 +0x32a

cdrage · 2020-07-07T20:09:37Z

pkg/kclient/deployments.go

+	})
+}
+
+func (c *Client) ListAllDeployments() (*appsv1.DeploymentList, error) {


Missing comments

cdrage · 2020-07-07T20:09:41Z

pkg/kclient/deployments.go

@@ -42,6 +44,18 @@ func getDeploymentCondition(status appsv1.DeploymentStatus, condType appsv1.Depl
 	return nil
 }

+func (c *Client) ListDeployments(appName string) (*appsv1.DeploymentList, error) {


Missing comments

cdrage · 2020-07-07T20:10:35Z

pkg/odo/cli/component/list.go

 	if len(lo.pathFlag) != 0 {
+
+		if experimental.IsExperimentalModeEnabled() {


Am I missing something or was this moved from somewhere else?

component list didn't support experimental mode yet

the --path flag support for experimental mode will be added as a follow up PR as the changes in this PR are already quite wide spread already

cdrage · 2020-07-07T20:10:55Z

pkg/odo/cli/component/list.go

+		var dpl *appsv1.DeploymentList
+		var err error
+
+		// TODO: wrap this into a component list for docker support


left a TODO here by mistake?

docker support marker for someone who would implement it

cdrage · 2020-07-07T20:11:25Z

pkg/odo/cli/component/list.go

-		components, err = component.List(lo.Client, lo.Application, lo.LocalConfigInfo)
-		if err != nil {
-			return errors.Wrapf(err, "failed to fetch components list")
+		klog.V(4).Infof("the components are %+v", components)


maybe remove this debug before merging

This was added by tomas like 8 months ago https://github.com/openshift/odo/blob/master/pkg/odo/cli/component/list.go#L143, I can remove this if you want

cdrage · 2020-07-07T20:11:35Z

pkg/odo/cli/component/list.go

 			if err != nil {
-				return err
+				return errors.Wrapf(err, "failed to fetch components list")


failed to fetch component list

cdrage · 2020-07-07T20:19:10Z

Tried again with the newest changes and I still get an issue:

~/openshift/foobar                                                                                                                                                                                                                                                               
▶ odo create nodejs --starter
Experimental mode is enabled, use at your own risk

Validation
 ✓  Checking devfile existence [76586ns]
 ✓  Checking devfile compatibility [1ms]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [1ms]
 ✓  Validating devfile component [338458ns]

Please use `odo push` command to create the component with source deployed

~/openshift/foobar                                                                                                                                                                                                                                                               
▶ odo push

Validation
 ✓  Validating the devfile [21778ns]

Syncing to component nodejs
 ✓  Checking file changes for pushing [451061ns]
 ✓  Syncing files to the component [364ms]

Executing devfile commands for component nodejs
 ✓  Executing install command "npm install" [1s]
 ✓  Executing run command "npm start" [2s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component

~/openshift/foobar                                                                                                                                                                                                                                                               
▶ odo list
Experimental mode is enabled, use at your own risk

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1591280]

goroutine 1 [running]:
github.com/openshift/odo/pkg/kclient.(*Client).ListDeployments(0x0, 0xc0007212d0, 0x3, 0x10, 0xc00066a720, 0xc0006d1100)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/kclient/deployments.go:50 +0xf0
github.com/openshift/odo/pkg/odo/cli/component.(*ListOptions).Run(0xc000051e40, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/cli/component/list.go:169 +0x1578
github.com/openshift/odo/pkg/odo/genericclioptions.GenericRun(0x1f73640, 0xc000051e40, 0xc0002bc000, 0x2fe0750, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/genericclioptions/runnable.go:31 +0x13c
github.com/openshift/odo/pkg/odo/cli/component.NewCmdList.func1(0xc0002bc000, 0x2fe0750, 0x0, 0x0)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/pkg/odo/cli/component/list.go:277 +0x5e
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).execute(0xc0002bc000, 0x2fe0750, 0x0, 0x0, 0xc0002bc000, 0x2fe0750)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:830 +0x2aa
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc000136000, 0x1d6cc10, 0xc0004ff620, 0x1)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:914 +0x2fb
github.com/openshift/odo/vendor/github.com/spf13/cobra.(*Command).Execute(...)
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/vendor/github.com/spf13/cobra/command.go:864
main.main()
        /home/wikus/syncthing/files/dev/go/src/github.com/openshift/odo/cmd/odo/odo.go:67 +0x32a

girishramnani · 2020-07-07T20:24:19Z

I followed the same approach

girish@MacBook-Pro bbb % odo create nodejs --starter
Experimental mode is enabled, use at your own risk

Validation
 ✓  Checking devfile existence [55782ns]
 ✓  Checking devfile compatibility [172449ns]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [168786ns]
 ✓  Validating devfile component [213798ns]

Please use `odo push` command to create the component with source deployed
girish@MacBook-Pro bbb % odo push                   

Validation
 ✓  Validating the devfile [126889ns]

Creating Kubernetes resources for component nodejs
 ✓  Waiting for component to start [4s]

Applying URL changes
 ✓  URLs are synced with the cluster, no changes are required.

Syncing to component nodejs
 ✓  Checking files for pushing [2ms]
 ✓  Syncing files to the component [4s]

Executing devfile commands for component nodejs
 ✓  Executing install command "npm install" [5s]
 ✓  Executing run command "npm start" [5s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component
girish@MacBook-Pro bbb % odo list
Experimental mode is enabled, use at your own risk

Devfile Components: 
APP     NAME       PROJECT     TYPE       REVISION
app     nodejs     asdf        nodejs     1

can you share any pre-steps you are performing?

girishramnani · 2020-07-07T20:30:48Z

Tried again on a kuberenetes cluster -

girish@MacBook-Pro bbb % odo create nodejs --starter
Experimental mode is enabled, use at your own risk

Validation
 ✓  Checking devfile existence [54923ns]
 ✓  Checking devfile compatibility [157220ns]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [169341ns]
 ✓  Validating devfile component [991819ns]

Please use `odo push` command to create the component with source deployed
girish@MacBook-Pro bbb % odo push

Validation
 ✓  Validating the devfile [33293ns]

Creating Kubernetes resources for component nodejs
 ✓  Waiting for component to start [1m]

Applying URL changes
 ✓  URLs are synced with the cluster, no changes are required.

Syncing to component nodejs
 ✓  Checking files for pushing [889725ns]
 ✓  Syncing files to the component [310ms]

Executing devfile commands for component nodejs
 ✓  Executing install command "npm install" [3s]
 ✓  Executing run command "npm start" [1s]

Pushing devfile component nodejs
 ✓  Changes successfully pushed to component
girish@MacBook-Pro bbb % odo list
Experimental mode is enabled, use at your own risk

Devfile Components: 
APP     NAME       PROJECT     TYPE       REVISION
app     nodejs     default     nodejs     1

girishramnani · 2020-07-07T22:32:09Z

@cdrage addressed all your comments

girishramnani · 2020-07-07T23:11:03Z

/retest

girishramnani · 2020-07-08T00:46:27Z

/retest

girishramnani · 2020-07-08T03:53:19Z

/retest

mik-dass · 2020-07-08T06:31:22Z

pkg/occlient/occlient.go

+	list, err := c.discoveryClient.ServerResourcesForGroupVersion(groupVersion)
+	if kerrors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, err
+	}
+
+	for _, resources := range list.APIResources {


We can use isResourceSupported() for checking the existence of a resource.

mik-dass · 2020-07-08T06:33:12Z

pkg/odo/cli/component/list.go

+		if util.CheckKubeConfigExist() {
+			klog.V(4).Infof("New Context")
+			lo.Context = genericclioptions.NewContext(cmd)
+		} else {


Some comments here

mik-dass · 2020-07-08T06:34:09Z

pkg/odo/cli/component/list.go

 	}
+
+	lo.Client = genericclioptions.Client(cmd)
+	dcSupport, err := lo.Client.IsDeploymentConfigSupported()


maybe assign to lo.hasDCSupport directly here

mik-dass · 2020-07-08T06:35:28Z

pkg/odo/cli/component/list.go

 	if len(lo.Application) != 0 && lo.allAppsFlag {
 		klog.V(4).Infof("either --app and --all-apps both provided or provided --all-apps in a folder has app, use --all-apps anyway")
 	}

+	if experimental.IsExperimentalModeEnabled() {


maybe store the value somewhere and use it here

mik-dass · 2020-07-08T06:37:26Z

pkg/odo/cli/component/list.go

+
+	if experimental.IsExperimentalModeEnabled() {
+
+		var dpl *appsv1.DeploymentList


something better than dpl

mik-dass · 2020-07-08T06:51:26Z

pkg/odo/cli/component/list.go

+			for _, comp := range dpl.Items {
+				app := comp.Labels[applabels.ApplicationLabel]
+				cmpType := comp.Labels[componentlabels.ComponentTypeLabel]
+				revision := comp.Annotations["deployment.kubernetes.io/revision"]


do we need to display the revision?

It allowed me to understand that my application was updated or not, does it reduce the UX?

I seems s2i components don't have this column. Can we have the component STATE instead?

mik-dass · 2020-07-08T07:01:48Z

tests/integration/devfile/cmd_devfile_push_test.go

+	Context("push with listing the devfile component", func() {
+
+		It("checks components in a specific app and all apps", func() {
+			helper.Chdir(currentWorkingDirectory)


do we need to change directory?

done, not changing the dir now

pkg/odo/genericclioptions/context.go

pkg/odo/cli/component/list.go

girishramnani · 2020-07-08T07:23:17Z

/retest

mik-dass · 2020-07-08T11:44:52Z

pkg/occlient/occlient.go

+	const Group = "apps.openshift.io"
+	const Version = "v1"
+
+	return c.isResourceSupported(Group, Version, "DeploymentConfig")


This seems to panic for me, when run against a s2i component in experimental mode, as the client is nil

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1c29be0] goroutine 1 [running]: github.com/openshift/odo/pkg/occlient.(*Client).isResourceSupported(0x0, 0x24f97a2, 0x11, 0x24ea377, 0x2, 0x24f7688, 0x10, 0xc000261300, 0x0, 0x0) /home/mrinaldas/go/src/github.com/openshift/odo/pkg/occlient/occlient.go:3341 +0xf0 github.com/openshift/odo/pkg/occlient.(*Client).IsDeploymentConfigSupported(0x0, 0x0, 0x0, 0x0) /home/mrinaldas/go/src/github.com/openshift/odo/pkg/occlient/occlient.go:3295 +0x8c github.com/openshift/odo/pkg/odo/cli/component.(*ListOptions).Complete(0xc0003ceac0, 0x24eafa4, 0x4, 0xc0006a3680, 0x39712d8, 0x0, 0x0, 0x0, 0x0)

Even fails for devfile components.

resolved, so we cannot create the Client only if the resource exists as thats a catch 22 situation.

cdrage

Code looks good! Thanks for the update @girishramnani but this LGTM.

/lgtm

girishramnani · 2020-07-22T06:27:38Z

/retest

mik-dass

/approve

openshift-ci-robot · 2020-07-22T06:36:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mik-dass

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~~ [mik-dass]

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

dharmit · 2020-07-22T06:51:10Z

/lgtm

dharmit · 2020-07-22T08:47:24Z

/lgtm

openshift-ci-robot requested review from cdrage and mohammedzee1000 July 7, 2020 19:25

girishramnani commented Jul 7, 2020

View reviewed changes

cdrage suggested changes Jul 7, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Jul 7, 2020

girishramnani requested review from dharmit, mik-dass and cdrage July 7, 2020 20:35

girishramnani force-pushed the list_components branch from b523541 to 66e0c7f Compare July 8, 2020 04:18

mik-dass reviewed Jul 8, 2020

View reviewed changes

pkg/odo/genericclioptions/context.go Outdated Show resolved Hide resolved

mik-dass reviewed Jul 8, 2020

View reviewed changes

pkg/odo/genericclioptions/context.go Show resolved Hide resolved

mik-dass reviewed Jul 8, 2020

View reviewed changes

pkg/odo/cli/component/list.go Outdated Show resolved Hide resolved

girishramnani force-pushed the list_components branch from 66e0c7f to 6a491b5 Compare July 8, 2020 08:25

mik-dass reviewed Jul 8, 2020

View reviewed changes

girishramnani force-pushed the list_components branch from 57ee711 to a366be0 Compare July 8, 2020 15:17

girishramnani added 10 commits July 21, 2020 23:19

resolved the panic

a321985

use client and dc support for specific cases

bac356b

resolved final tests and odo list now worked for unpushed components

56958ec

resolved failing unit tests

16b7f7f

update the component adapter in other places

c01ea74

resolved all comments

864acda

removed the created context at the end of the test

5a7c1cc

addressed tomas's comment

7560843

resolved all comments

303ef14

updated all devfiles with proper name

71c34b4

girishramnani force-pushed the list_components branch from 21ec6c8 to 71c34b4 Compare July 21, 2020 17:50

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

resolved rebase

2333dae

cdrage approved these changes Jul 21, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 21, 2020

mik-dass approved these changes Jul 22, 2020

View reviewed changes

openshift-ci-robot assigned mik-dass Jul 22, 2020

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

addressed a missed error and more validation in the tests

2905e29

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2020

openshift-ci-robot assigned dharmit Jul 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2020

use random cmpName for tests

363b4eb

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 22, 2020

openshift-merge-robot merged commit f40bf97 into redhat-developer:master Jul 22, 2020

		if len(lo.pathFlag) != 0 {

		if experimental.IsExperimentalModeEnabled() {


		if experimental.IsExperimentalModeEnabled() {

		var dpl *appsv1.DeploymentList

List created components for devfile v2 #3505

List created components for devfile v2 #3505

Conversation

girishramnani commented Jul 7, 2020 • edited

Choose a reason for hiding this comment

cdrage 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

cdrage commented Jul 7, 2020

girishramnani commented Jul 7, 2020

girishramnani commented Jul 7, 2020

girishramnani commented Jul 7, 2020

girishramnani commented Jul 7, 2020

girishramnani commented Jul 8, 2020

girishramnani commented Jul 8, 2020

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

girishramnani Jul 8, 2020 • 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 Jul 8, 2020 • edited

Choose a reason for hiding this comment

girishramnani commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

girishramnani commented Jul 22, 2020

mik-dass left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 22, 2020

dharmit commented Jul 22, 2020

dharmit commented Jul 22, 2020

girishramnani commented Jul 7, 2020 •

edited

girishramnani Jul 8, 2020 •

edited

girishramnani Jul 8, 2020 •

edited