Odo test cmd for devfile 2.0 #3400

yangcao77 · 2020-06-22T19:07:23Z

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 user-story
/area devfileV2

What does does this PR do / why we need it:
odo test will run test command specified in the command that belongs to the group test defined in the devfile

odo uses this logic to decide which test command to run on odo test:

if only one test command is define, use that command regardless of whether that command with isDefault is set to true or not.
if more than one test command, execute the one with isDefault set to true
odo test --test-command <command name> will execute the command with the given command name regardless of a default test command is defined or not.
if more than one test command but no default is defined(or if multiple default commands are defined), error out if --test-command is not use.

Which issue(s) this PR fixes:

Fixes #3284

How to test changes / Special notes to the reviewer:
Automation:
run make test-cmd-devfile-test and make test-cmd-docker-devfile-test

Manual:
(The following shows docker scenario, kube scenario should be similar)
Use the devfile.yaml within this PR: devfile-with-testgroup.yaml

run odo create and odo push.
run odo test
docker exec -it <containerID> ls -la /projects/nodejs-web-app/ should see test1 directory exist

run odo test --test-command test2
docker exec -it <containerID> ls -la /projects/nodejs-web-app/ should see test2 directory exist

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

yangcao77 · 2020-06-22T19:09:53Z

Mark as a draft pr since this PR is blocked by #3291
Copied some functions from #3291 in order to make it work. Need to rebase after #3291 gets merged.

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

mik-dass · 2020-07-02T14:29:52Z

pkg/devfile/adapters/docker/component/adapter.go

@@ -140,6 +141,27 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
 	return nil
 }

+func (a Adapter) Test(testcmd versionsCommon.DevfileCommand, show bool) (err error) {


Please add some comments for the function

mik-dass · 2020-07-02T14:37:31Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+	componentExists := utils.ComponentExists(a.Client, a.ComponentName)
+	if !componentExists {
+		return fmt.Errorf("component does not exist, a valid component is required to run 'odo test'")
+	}


Do we really need this call since we are getting the pod just after it? Won't that be enough?

This checks if the deployment exist; and we should show different error message if there is a problem when getting pod.

But on a slow cluster both calls will cause performance problems. Since without the pod, it won't run anyways, just check for the pod and error out with a message like the component is not running.....

Since this PR does both docker and kube; docker logic cannot be applied with the above logic since an odo component in docker can have multiple containers. So if a devfile has 2 components, that translates to 2 containers in docker. But if one of the containers is down; we cannot say a component exist with at least one container in our example.

I can honestly see where your argument is but i think even odo watch and odo push have these logic which can be refactored more optimally.

Since this PR does both docker and kube; docker logic cannot be applied with the above logic since an odo component in docker can have multiple containers.

For docker, we can write it separately in the docker adapter. Logic in the Kubernetes should be independent of that. Otherwise performance of kubernetes components will be impacted unnecessarily.

@yangcao77 with #3400 (comment) implemented, we can remove this call as the pod check is necessary anyways. So only check for the pod and if not running, just error out with a message like component is not running

updated the Kubernetes logic

mik-dass · 2020-07-02T14:43:27Z

pkg/odo/cli/component/test.go

+		Short:   "Run test command defined in devfile",
+		Long:    "Run test command defined in devfile",


mik-dass · 2020-07-02T14:46:31Z

vendor/k8s.io/kubectl/go.sum

+k8s.io/api v0.17.1 h1:i46MidoDOE9tvQ0TTEYggf3ka/pziP1+tHI/GFVeJao=
 k8s.io/api v0.17.1/go.mod h1:zxiAc5y8Ngn4fmhWUtSxuUlkfz1ixT7j9wESokELzOg=
+k8s.io/apimachinery v0.17.1 h1:zUjS3szTxoUjTDYNvdFkYt2uMEXLcthcbp+7uZvWhYM=
 k8s.io/apimachinery v0.17.1/go.mod h1:b9qmWdKlLuU9EBh+06BtLcSf/Mu89rWL33naRxs1uZg=
 k8s.io/cli-runtime v0.17.1/go.mod h1:e5847Iy85W9uWH3rZofXTG/9nOUyGKGTVnObYF7zSik=
+k8s.io/client-go v0.17.1 h1:LbbuZ5tI7OYx4et5DfRFcJuoojvpYO0c7vps2rgJsHY=


I think these changes need to be removed

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

yangcao77 · 2020-07-14T14:53:35Z

/unhold

mik-dass

Works for me
/lgtm

cdrage · 2020-07-14T18:17:12Z

/hold

Honestly, we haven't had much discussion with regards to this command in the issue: #3284

Perhaps we can make 100% sure we want this command before we merge?

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

yangcao77 · 2020-07-15T16:12:41Z

/retest

yangcao77 · 2020-07-15T17:30:46Z

/retest

maysunfaisal · 2020-07-15T17:41:51Z

re-adding lgtm after rebase
/lgtm

yangcao77 · 2020-07-16T00:27:34Z

@cdrage Do you still have any questions or concerns that you want to discuss?
Can I get an approve label for this PR?

kadel · 2020-07-16T11:16:07Z

/hold cancel
/approve

openshift-ci-robot · 2020-07-16T11:16:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [kadel]

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

kadel · 2020-07-16T11:17:10Z

/retest

yangcao77 · 2020-07-16T18:19:37Z

/retest

openshift-bot · 2020-07-16T20:08:01Z

/retest

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

Stephanie added 9 commits June 15, 2020 10:56

first commit to save change

9337988

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

able to run odo test

32db684

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

Merge branch 'master' of github.com:openshift/odo into odotestcmd

067b2ca

add test in adapters

dad2b42

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

Merge branch 'master' of github.com:openshift/odo into odotestcmd

b6b3f39

test works

b4c6954

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

Merge branch 'master' of github.com:openshift/odo into odotestcmd

12dbc29

Merge branch 'master' of github.com:openshift/odo into odotestcmd

3262b31

add integration test

90ce521

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

openshift-ci-robot added kind/user-story An issue of user-story kind area/devfileV2 labels Jun 22, 2020

openshift-ci-robot requested review from maysunfaisal and mik-dass June 22, 2020 19:07

yangcao77 marked this pull request as draft June 22, 2020 19:08

openshift-ci-robot added 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 22, 2020

Merge branch 'master' of github.com:openshift/odo into odotestcmd

c1a4fb4

kadel removed the area/devfileV2 label Jun 25, 2020

Merge branch 'master' of github.com:openshift/odo into odotestcmd

4a7a3d7

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

rebase

575ef17

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

yangcao77 marked this pull request as ready for review June 29, 2020 15:40

openshift-ci-robot 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 29, 2020

fix integration test

8da007a

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

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

mik-dass reviewed Jul 2, 2020

View reviewed changes

Stephanie added 2 commits July 14, 2020 09:22

Merge branch 'master' of github.com:openshift/odo into odotestcmd

7e8939c

address review comments

d1eeda1

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

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

Stephanie added 3 commits July 14, 2020 09:40

simplify the error return

6c43b35

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

test command should have 0 args

fce43ce

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

add test in the docker test description

0c47c7d

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 Jul 14, 2020

mik-dass approved these changes Jul 14, 2020

View reviewed changes

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

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

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 Jul 14, 2020

Stephanie added 2 commits July 15, 2020 11:01

resolve conflict

399efdf

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

modify trivis.yaml file

53ea5f1

Signed-off-by: Stephanie <stephanie.cao@ibm.com>

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

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

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 Jul 16, 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 16, 2020

openshift-merge-robot merged commit 81adbcc into redhat-developer:master Jul 17, 2020

maysunfaisal mentioned this pull request Aug 5, 2020

PreStop Event Feature #3631

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Odo test cmd for devfile 2.0 #3400

Odo test cmd for devfile 2.0 #3400

yangcao77 commented Jun 22, 2020 •

edited

Loading

yangcao77 commented Jun 22, 2020

mik-dass Jul 2, 2020

yangcao77 Jul 6, 2020

mik-dass Jul 2, 2020

yangcao77 Jul 6, 2020

mik-dass Jul 7, 2020 •

edited

Loading

maysunfaisal Jul 7, 2020

mik-dass Jul 8, 2020 •

edited

Loading

yangcao77 Jul 9, 2020

mik-dass Jul 2, 2020

yangcao77 Jul 6, 2020

mik-dass Jul 2, 2020

yangcao77 Jul 6, 2020

yangcao77 commented Jul 14, 2020

mik-dass left a comment

cdrage commented Jul 14, 2020

yangcao77 commented Jul 15, 2020

yangcao77 commented Jul 15, 2020

maysunfaisal commented Jul 15, 2020

yangcao77 commented Jul 16, 2020

kadel commented Jul 16, 2020

openshift-ci-robot commented Jul 16, 2020

kadel commented Jul 16, 2020

yangcao77 commented Jul 16, 2020

openshift-bot commented Jul 16, 2020

		Short: "Run test command defined in devfile",
		Long: "Run test command defined in devfile",

Odo test cmd for devfile 2.0 #3400

Odo test cmd for devfile 2.0 #3400

Conversation

yangcao77 commented Jun 22, 2020 • edited Loading

yangcao77 commented Jun 22, 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

mik-dass Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Jul 8, 2020 • edited Loading

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

yangcao77 commented Jul 14, 2020

mik-dass left a comment

Choose a reason for hiding this comment

cdrage commented Jul 14, 2020

yangcao77 commented Jul 15, 2020

yangcao77 commented Jul 15, 2020

maysunfaisal commented Jul 15, 2020

yangcao77 commented Jul 16, 2020

kadel commented Jul 16, 2020

openshift-ci-robot commented Jul 16, 2020

kadel commented Jul 16, 2020

yangcao77 commented Jul 16, 2020

openshift-bot commented Jul 16, 2020

yangcao77 commented Jun 22, 2020 •

edited

Loading

mik-dass Jul 7, 2020 •

edited

Loading

mik-dass Jul 8, 2020 •

edited

Loading