Adds delete function for devfiles support. #2763

mik-dass · 2020-03-26T05:29:45Z

Signed-off-by: mik-dass mrinald7@gmail.com

What type of PR is this?

/kind feature

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

This PR also adds support for all flag. The all flag deletes the env folder present in the .odo folder.

Which issue(s) this PR fixes:

Fixes #2591

How to test changes / Special notes to the reviewer:

execute odo push on a devfile component
execute odo delete -f and check if the devfile component is deleted from the cluster

Test the --all flag

execute odo push on a devfile component
create a url
execute odo delete -f --all and check if the devfile component is deleted from the cluster. Also check if the env folder is deleted from the .odo folder

codecov · 2020-03-26T08:30:44Z

Codecov Report

Merging #2763 into master will increase coverage by 0.14%.
The diff coverage is 57.89%.

@@            Coverage Diff             @@
##           master    #2763      +/-   ##
==========================================
+ Coverage   43.53%   43.67%   +0.14%     
==========================================
  Files          94       95       +1     
  Lines        8743     8762      +19     
==========================================
+ Hits         3806     3827      +21     
- Misses       4569     4571       +2     
+ Partials      368      364       -4

Impacted Files	Coverage Δ
pkg/testingutil/deployments.go	`0.00% <0.00%> (ø)`
...g/devfile/adapters/kubernetes/component/adapter.go	`27.88% <100.00%> (+0.93%)`	⬆️
pkg/kclient/deployments.go	`40.24% <100.00%> (+5.57%)`	⬆️
pkg/watch/watch.go	`69.41% <0.00%> (+5.88%)`	⬆️

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 484a32d...f21452b. Read the comment docs.

kadel · 2020-04-06T18:27:26Z

/approve

openshift-ci-robot · 2020-04-06T18:28:52Z

[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

This PR also adds support for all flag. The all flag deletes the env folder present in the .odo folder. Signed-off-by: mik-dass <mrinald7@gmail.com>

amitkrout · 2020-04-07T10:51:54Z

tests/integration/devfile/cmd_devfile_delete_test.go

+		os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml"))
+
+		// Devfile commands require experimental mode to be set
+		helper.CmdShouldPass("odo", "preference", "set", "Experimental", "true")


@mik-dass if you dont mind, i shall make this change for the push integration tests when i am putting up a PR

Yup, I like this and I already did that in my odo create PR, we should apply that to all integration tests under tests/integration/devfile/

@mik-dass if you dont mind, i shall make this change for the push integration tests when i am putting up a PR

Sure no problem

amitkrout

UTs and Integration test looks good to me, however it would be great if dev folks including IBM folks will have a look and share their comments

Ping @GeekArthur @johnmcollier @elsony @dharmit @girishramnani

dharmit · 2020-04-07T11:36:16Z

@mik-dass @kadel

odo delete --all doesn't delete devfile.yaml. Is that by design?
odo delete --all doesn't delete .odo/odo-file-index.json. I think that file should be removed, no?

Instead of devfile component, I think we should use the name of the component under deletion:

$  odo delete --all
? Are you sure you want to delete the devfile component? No  <------ here
 ✗  Aborting deletion of component
? Are you sure you want to delete env folder? No
 ✗  Aborting deletion of env folder

$ odo delete
? Are you sure you want to delete the devfile component? No  <------ here
 ✗  Aborting deletion of component

This is not related to this PR, but odo url create asks me to specify value for host but there's no example for it in odo url create -h !!

$ odo url create
 ✗  host must be provided in order to create ingress

$ odo url create -h
Create a URL for a component. The created URL can be used to access the specified component from outside the OpenShift cluster.

Usage:
  odo url create [url name] [flags]

Examples:
  # Create a URL with a specific name by automatically detecting the port used by the component
  odo url create example
  # Create a URL for the current component with a specific port
  odo url create --port 8080
  
  # Create a URL with a specific name and port
  odo url create example --port 8080

Flags:
      --context string      Use given context directory as a source for component settings
      --devfile string      Path to a devfile.yaml (default "./devfile.yaml")
  -h, --help                Help for create
      --host string         Cluster ip for this URL
      --now                 Push changes to the cluster immediately
      --port int            Port number for the url of the component, required in case of components which expose more than one service port (default -1)
      --secure              Creates a secure https url
      --tls-secret string   Tls secret name for the url of the component if the user bring his own tls secret

Additional Flags:
  -v, --v Level              Log level for V logs. Level varies from 0 to 9 (default 0).
      --vmodule moduleSpec   Comma-separated list of pattern=N settings for file-filtered logging

EDIT: Created #2832 for missing odo url create -h documentation.

mik-dass · 2020-04-07T12:02:48Z

odo delete --all doesn't delete devfile.yaml. Is that by design?

Yes #2591 (comment)

odo delete --all doesn't delete .odo/odo-file-index.json. I think that file should be removed, no?

Currently, I was just deleting the env folder only. I will update the PR to delete the entire .odo folder.

kadel · 2020-04-07T13:43:51Z

odo delete --all doesn't delete .odo/odo-file-index.json. I think that file should be removed, no?

Currently, I was just deleting the env folder only. I will update the PR to delete the entire .odo folder

@dharmit
Could we fix that in another PR? It would be good to have at least odo delete working for next release

cdrage

LGTM!

Amazing how there are more tests than actual code (which is awesome!)

/lgtm

maysunfaisal · 2020-04-07T14:51:08Z

Good change, tested it and worked well!

Just had some thoughts -

Maysuns-MacBook-Pro:spring-devfile maysun$ odo delete -a
? Are you sure you want to delete the devfile component? Yes
 ✓  Deleting devfile component maysunspring [449ms]
 ✓  Successfully deleted component
? Are you sure you want to delete env folder? Yes
 ✓  Successfully deleted env file

It would be nice if the prompt told me the component name in the question as well, esp with the context flag. Thoughts?

johnmcollier

/lgtm Overall changes look good to me and work as expected.

I left a couple comments on some suggestions to consider in the future, as we add more devfile support to odo.

johnmcollier · 2020-04-07T15:01:47Z

pkg/odo/cli/component/devfile.go

@@ -137,3 +137,40 @@ func getNamespace() (string, error) {
 	Namespace := envInfo.GetNamespace()
 	return Namespace, nil
 }
+
+// DevfileComponentDelete deletes the devfile component
+func (do *DeleteOptions) DevfileComponentDelete() error {


Something to keep in mind: Rather than having a single devfile.go that contains the functions for push, delete, etc. We should probably separate them out into their own files.

👍 We should have a look into it.

johnmcollier · 2020-04-07T15:09:58Z

pkg/odo/cli/component/delete.go

@@ -74,6 +98,39 @@ func (do *DeleteOptions) Run() (err error) {
 	glog.V(4).Infof("component delete called")
 	glog.V(4).Infof("args: %#v", do)

+	if experimental.IsExperimentalModeEnabled() && util.CheckPathExists(do.devfilePath) {


Another thought I've had: We've been using if blocks in the Complete/Validate/Run sections of each CLI command for devfile support, I wonder if we should move them into devfileComplete/devfileValidate/devfileRun sections to avoid cluttering the code?

Sounds good 👍 I have moved the Run() part for devfiles into a seperate function #2842 for now.

openshift-bot · 2020-04-07T15:57:33Z

/retest

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

openshift-bot · 2020-04-07T16:10:36Z

/retest

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

openshift-bot · 2020-04-07T23:31:34Z

/retest

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

maysunfaisal · 2020-04-07T23:32:00Z

/test v4.1-integration-e2e-benchmark

openshift-bot · 2020-04-07T23:44:34Z

/retest

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

mik-dass · 2020-04-08T05:31:52Z

It would be nice if the prompt told me the component name in the question as well, esp with the context flag. Thoughts?

@maysunfaisal @dharmit I have addressed the above suggestions in a follow up PR #2842. Please have a look :)

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 Mar 26, 2020

openshift-ci-robot requested review from girishramnani and mohammedzee1000 March 26, 2020 05:30

mik-dass force-pushed the dev_del branch from 13e6f44 to 10b4c59 Compare March 26, 2020 08:30

mik-dass force-pushed the dev_del branch 2 times, most recently from e571a43 to aefce34 Compare March 26, 2020 10:00

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 27, 2020

mik-dass force-pushed the dev_del branch from aefce34 to da7f1e1 Compare March 27, 2020 05:01

elsony mentioned this pull request Mar 27, 2020

devfile support for odo delete #2591

Closed

4 tasks

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

mik-dass force-pushed the dev_del branch from da7f1e1 to cdaaa75 Compare March 30, 2020 04:17

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

mik-dass changed the title ~~[WIP] Adds delete function for devfiles support.~~ Adds delete function for devfiles support. Mar 30, 2020

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 Mar 30, 2020

mik-dass changed the title ~~Adds delete function for devfiles support.~~ [WIP] Adds delete function for devfiles support. Mar 30, 2020

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 Mar 30, 2020

mik-dass force-pushed the dev_del branch 2 times, most recently from 7aa5735 to ef267e0 Compare March 31, 2020 04:58

mik-dass changed the title ~~[WIP] Adds delete function for devfiles support.~~ Adds delete function for devfiles support. Mar 31, 2020

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 Mar 31, 2020

mik-dass force-pushed the dev_del branch from ef267e0 to 71fb39f Compare March 31, 2020 05:16

adisky mentioned this pull request Apr 2, 2020

odo support for quarkus #2800

Closed

5 tasks

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

mik-dass force-pushed the dev_del branch from 71fb39f to 50f67d0 Compare April 6, 2020 03:42

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

mik-dass force-pushed the dev_del branch from 50f67d0 to a68bfff Compare April 6, 2020 04:33

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

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

mik-dass added 2 commits April 7, 2020 09:24

Adds delete function for devfiles support.

87bd428

This PR also adds support for all flag. The all flag deletes the env folder present in the .odo folder. Signed-off-by: mik-dass <mrinald7@gmail.com>

Adds integration test for devfile delete

f21452b

mik-dass force-pushed the dev_del branch from a68bfff to f21452b Compare April 7, 2020 03:58

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

amitkrout reviewed Apr 7, 2020

View reviewed changes

cdrage approved these changes Apr 7, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Apr 7, 2020

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

johnmcollier approved these changes Apr 7, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Apr 7, 2020

openshift-merge-robot merged commit cef105b into redhat-developer:master Apr 8, 2020

mik-dass deleted the dev_del branch April 8, 2020 04:15

mik-dass mentioned this pull request Apr 8, 2020

Improves odo delete for devfiles #2842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds delete function for devfiles support. #2763

Adds delete function for devfiles support. #2763

mik-dass commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited

kadel commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

amitkrout Apr 7, 2020

mik-dass Apr 7, 2020

maysunfaisal Apr 7, 2020

maysunfaisal Apr 7, 2020

GeekArthur Apr 7, 2020

mik-dass Apr 8, 2020

amitkrout left a comment

dharmit commented Apr 7, 2020 •

edited

mik-dass commented Apr 7, 2020

kadel commented Apr 7, 2020 •

edited

cdrage left a comment

maysunfaisal commented Apr 7, 2020

johnmcollier left a comment

johnmcollier Apr 7, 2020

mik-dass Apr 8, 2020 •

edited

johnmcollier Apr 7, 2020

mik-dass Apr 8, 2020 •

edited

openshift-bot commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

maysunfaisal commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

mik-dass commented Apr 8, 2020 •

edited

Adds delete function for devfiles support. #2763

Adds delete function for devfiles support. #2763

Conversation

mik-dass commented Mar 26, 2020 • edited

codecov bot commented Mar 26, 2020 • edited

Codecov Report

kadel commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 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

amitkrout left a comment

Choose a reason for hiding this comment

dharmit commented Apr 7, 2020 • edited

mik-dass commented Apr 7, 2020

kadel commented Apr 7, 2020 • edited

cdrage left a comment

Choose a reason for hiding this comment

maysunfaisal commented Apr 7, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Apr 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Apr 8, 2020 • edited

Choose a reason for hiding this comment

openshift-bot commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

maysunfaisal commented Apr 7, 2020

openshift-bot commented Apr 7, 2020

mik-dass commented Apr 8, 2020 • edited

mik-dass commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited

dharmit commented Apr 7, 2020 •

edited

kadel commented Apr 7, 2020 •

edited

mik-dass Apr 8, 2020 •

edited

mik-dass Apr 8, 2020 •

edited

mik-dass commented Apr 8, 2020 •

edited