Remove unnecessary devfile flag and update tests #3762

maysunfaisal · 2020-08-17T18:04:31Z

Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com

What type of PR is this?

/kind bug
/kind cleanup

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

only odo create needs the devfile flag to copy contents from the specified flag to project dir/context dir devfile.yaml
devfile is always devfile.yaml, so no point in having a devfile flag for non create commands
commands listed in the issue do not require --devfile flag
there were some bugs when using --context and --devfile flags for the command listed, they have been fixed
updated integration tests

Which issue(s) this PR fixes:

Fixes #3661

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

Run the commands with the devfile flag; it should err out
Run the commands with the context flag, it should work

maysunfaisal · 2020-08-17T18:07:57Z

@cdrage @GeekArthur can you help review this PR? Thx

pkg/odo/cli/component/exec.go

mik-dass · 2020-08-19T11:18:06Z

pkg/odo/cli/debug/info.go

@@ -46,7 +45,7 @@ func NewInfoOptions() *InfoOptions {

 // Complete completes all the required options for port-forward cmd.
 func (o *InfoOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) {
-	if experimental.IsExperimentalModeEnabled() && util.CheckPathExists(o.DevfilePath) {


Why was the other check removed? This will trigger NewDevfileContext even for s2i components.

[mrinaldas@localhost project]$ odo create nodejs --s2i Experimental mode is enabled, use at your own risk Validation ✓ Validating component [2s] Please use `odo push` command to create the component with source deployed [mrinaldas@localhost project]$ odo debug info ✗ The current directory does not represent an odo component. Use 'odo create' to create component here or switch to directory with a component

I didnt realize we had a s2i flag for experimental, I was wondering what the point of the check was. Thanks for showing it out, will update it!

mik-dass · 2020-08-19T11:19:08Z

pkg/odo/cli/debug/portforward.go

@@ -76,7 +76,7 @@ func (o *PortForwardOptions) Complete(name string, cmd *cobra.Command, args []st

 	o.isExperimental = experimental.IsExperimentalModeEnabled()

-	if o.isExperimental && util.CheckPathExists(o.DevfilePath) {


why was the other check removed?

mik-dass · 2020-08-19T11:23:21Z

pkg/odo/cli/url/delete.go

@@ -41,7 +40,7 @@ func NewURLDeleteOptions() *URLDeleteOptions {
 // Complete completes URLDeleteOptions after they've been Deleted
 func (o *URLDeleteOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) {

-	if experimental.IsExperimentalModeEnabled() && util.CheckPathExists(o.DevfilePath) {


removing this check will make odo url delete not work for s2i components

[mrinaldas@localhost project]$ odo create nodejs --s2i Experimental mode is enabled, use at your own risk Validation ✓ Validating component [1s] Please use `odo push` command to create the component with source deployed [mrinaldas@localhost project]$ odo url create example ✓ URL example created for component: nodejs-project-vwui To apply the URL configuration changes, please use `odo push` [mrinaldas@localhost project]$ odo url delete example ✗ The current directory does not represent an odo component. Use 'odo create' to create component here or switch to directory with a component

mik-dass · 2020-08-19T11:24:59Z

tests/integration/devfile/cmd_devfile_debug_test.go

@@ -67,20 +67,20 @@ var _ = Describe("odo devfile debug command tests", func() {
 			helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), projectDirPath)
 			helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-debugrun.yaml"), filepath.Join(projectDirPath, "devfile-with-debugrun.yaml"))
 			helper.RenameFile("devfile-with-debugrun.yaml", "devfile.yaml")
-			helper.CmdShouldPass("odo", "push", "--debug")


Since we are using context, I think we should also remove helper.Chdir(context) from beforeEach

it doesn't matter because the context path here is the full path to the component but nevertheless if i remove helper.Chdir(context), I need to update a bunch of statements because we're copying devfiles from the repo to the context.

And it looks like most of the other tests are doing this way ie; helper.Chdir(context) and using --context; so dont want the test to be inconsistent in the way its written. We should be updating all the tests where we use context to not chdir to the dir if we want to do it!

it doesn't matter because the context path here is the full path to the component but nevertheless if i remove helper.Chdir(context), I need to update a bunch of statements because we're copying devfiles from the repo to the context.

We have had problems before where the context flag wasn't working but it was never detected because we were changing into the context directory. In such cases, the current directory might be used even though the context was passed.

We should be updating all the tests where we use context to not chdir to the dir if we want to do it!

Yes, we need this behavior to change and need a issue to track it.

I have opened an issue here #3801 and yes ideally we should not do it

Thanks 👍

maysunfaisal · 2020-08-20T01:06:19Z

/retest

maysunfaisal · 2020-08-20T02:17:43Z

/retest

codecov · 2020-08-20T05:48:16Z

Codecov Report

Merging #3762 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3762   +/-   ##
=======================================
  Coverage   44.19%   44.19%           
=======================================
  Files         141      141           
  Lines       13600    13600           
=======================================
  Hits         6010     6010           
  Misses       7007     7007           
  Partials      583      583

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 68bc98b...c54e8af. Read the comment docs.

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

maysunfaisal · 2020-08-27T00:56:15Z

/retest

maysunfaisal · 2020-08-27T22:33:39Z

/retest

mik-dass

Tests are green
/lgtm

kadel · 2020-08-31T15:47:21Z

/approve

openshift-ci-robot · 2020-08-31T15:47:38Z

[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

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Aug 17, 2020

openshift-ci-robot requested review from cdrage and GeekArthur August 17, 2020 18:04

maysunfaisal force-pushed the 3661-1 branch from 17456ee to fa83f77 Compare August 17, 2020 18:07

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

maysunfaisal force-pushed the 3661-1 branch from 2ed11f6 to f4440e7 Compare August 18, 2020 20:55

mik-dass reviewed Aug 19, 2020

View reviewed changes

pkg/odo/cli/component/exec.go Show resolved Hide resolved

mik-dass reviewed Aug 19, 2020

View reviewed changes

maysunfaisal force-pushed the 3661-1 branch from c07cf1f to 7d2cf25 Compare August 19, 2020 18:23

maysunfaisal and others added 3 commits August 26, 2020 13:08

Remove unnecessary devfile flag and update tests

b7bd048

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Fix url delete for --devfile flag

e0e851a

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

PR Feedback for devfile flag

c54e8af

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>

maysunfaisal force-pushed the 3661-1 branch from 7d2cf25 to c54e8af Compare August 26, 2020 18:01

mik-dass approved these changes Aug 28, 2020

View reviewed changes

openshift-ci-robot assigned mik-dass Aug 28, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 28, 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 Aug 31, 2020

openshift-merge-robot merged commit ce2b6d0 into redhat-developer:master Aug 31, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary devfile flag and update tests #3762

Remove unnecessary devfile flag and update tests #3762

maysunfaisal commented Aug 17, 2020

maysunfaisal commented Aug 17, 2020

mik-dass Aug 19, 2020

maysunfaisal Aug 19, 2020

mik-dass Aug 19, 2020

maysunfaisal Aug 19, 2020

mik-dass Aug 19, 2020

maysunfaisal Aug 19, 2020

mik-dass Aug 19, 2020 •

edited

Loading

maysunfaisal Aug 19, 2020 •

edited

Loading

mik-dass Aug 20, 2020 •

edited

Loading

maysunfaisal Aug 20, 2020

mik-dass Aug 20, 2020

maysunfaisal commented Aug 20, 2020

maysunfaisal commented Aug 20, 2020

codecov bot commented Aug 20, 2020 •

edited

Loading

maysunfaisal commented Aug 27, 2020

maysunfaisal commented Aug 27, 2020

mik-dass left a comment •

edited

Loading

kadel commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

		@@ -76,7 +76,7 @@ func (o PortForwardOptions) Complete(name string, cmd cobra.Command, args []st

		o.isExperimental = experimental.IsExperimentalModeEnabled()

		if o.isExperimental && util.CheckPathExists(o.DevfilePath) {

Remove unnecessary devfile flag and update tests #3762

Remove unnecessary devfile flag and update tests #3762

Conversation

maysunfaisal commented Aug 17, 2020

maysunfaisal commented Aug 17, 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

mik-dass Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

maysunfaisal Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

mik-dass Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal commented Aug 20, 2020

maysunfaisal commented Aug 20, 2020

codecov bot commented Aug 20, 2020 • edited Loading

Codecov Report

maysunfaisal commented Aug 27, 2020

maysunfaisal commented Aug 27, 2020

mik-dass left a comment • edited Loading

Choose a reason for hiding this comment

kadel commented Aug 31, 2020

openshift-ci-robot commented Aug 31, 2020

mik-dass Aug 19, 2020 •

edited

Loading

maysunfaisal Aug 19, 2020 •

edited

Loading

mik-dass Aug 20, 2020 •

edited

Loading

codecov bot commented Aug 20, 2020 •

edited

Loading

mik-dass left a comment •

edited

Loading