Application Deletion will show what will be deleted #278

surajnarwade · 2018-03-27T16:26:08Z

Fixes #204

surajnarwade · 2018-03-28T09:16:35Z

@containscafeine @kadel , review needed :)

concaf · 2018-03-28T15:00:02Z

So, we need to see this before asking for the confirmation, not after that.
Currently,

$ ocdev application delete app5
Are you sure you want to delete the application: app5? [y/N] y
Component nodejs will be deleted:
This component is not externally exposed
This component used storage and it will be removed
Component nodejs2 will be deleted:
This component is not externally exposed
This component used storage and it will be removed
Component nodejs3 will be deleted:
This component is not externally exposed
This component used storage and it will be removed
Component nodejs4 will be deleted:
This component is not externally exposed
This component used storage and it will be removed
Component nodejs5 will be deleted:
This component is not externally exposed
This component used storage and it will be removed
Deleted application: app5

concaf · 2018-03-28T15:07:36Z

Also, my nodejs component did not use any storage, it should not the message.
We also need to print the storage name if it's being used

surajnarwade · 2018-03-29T06:31:23Z

@containscafeine when I run,

 $ ocdev storage list
deploymentconfigs/nodejs

I didn't get this

surajnarwade · 2018-03-29T06:35:04Z

I have updated code in such way that it will show information before confirmation

kadel · 2018-03-29T12:23:34Z

cmd/utils.go

+	"github.com/redhat-developer/ocdev/pkg/url"
+)
+
+func printRequiredInfo(appName string, client *occlient.Client) error {


This output is specific to delete operation. It might be good to indicate that in the function name
How about printDeleteAppInfo

also in all other functions, we are taking client as the first argument, it would be good to stick to that convention

and comment for that function wouldn't hurt either ;-)

surajnarwade · 2018-04-02T07:27:59Z

@kadel @containscafeine one more review needed :)

kadel · 2018-04-03T08:51:44Z

cmd/utils.go

+		if len(appUrl) != 0 {
+			fmt.Println("This component is externally exposed, and the URL will be removed")
+		} else {
+			fmt.Println("This component is not externally exposed")


it might be better to not show anything. To make it consistent with how we display storage.

concaf · 2018-04-05T12:09:46Z

cmd/application.go

@@ -94,6 +94,11 @@ var applicationDeleteCmd = &cobra.Command{
 		if applicationForceDeleteFlag {
 			confirmDeletion = "y"
 		} else {
+			err := printDeleteAppInfo(client, appName)


this should be before the if statement

concaf · 2018-04-05T12:16:55Z

cmd/utils.go

+func printDeleteAppInfo(client *occlient.Client, appName string) error {
+	componentList, err := component.List(client)
+	if err != nil {
+		return err


let's wrap the error

concaf · 2018-04-05T12:17:08Z

cmd/utils.go

+		fmt.Println("Component", cmpnt.Name, "will be deleted:")
+		appUrl, err := url.List(client, cmpnt.Name, appName)
+		if err != nil {
+			return err


concaf · 2018-04-05T12:18:03Z

cmd/utils.go

+			fmt.Println("This component is not externally exposed")
+
+		}
+		appStore, _ := storage.List(client, &occlient.VolumeConfig{


why are we ignoring the error here?

good catch @containscafeine

kadel · 2018-04-05T12:23:20Z

cmd/utils.go

+			fmt.Println("This component is not externally exposed")
+
+		}
+		appStore, _ := storage.List(client, &occlient.VolumeConfig{


good catch @containscafeine

surajnarwade · 2018-04-10T09:34:44Z

@kadel done with requested changes

kadel · 2018-04-10T12:46:57Z

cmd/application.go

@@ -75,6 +75,13 @@ var applicationDeleteCmd = &cobra.Command{
 		appName := args[0]
 		var confirmDeletion string

+		// Print App Information which will be deleted
+		err := printDeleteAppInfo(client, appName)
+		if err != nil {


you can replace with
checkError(err, "")

kadel · 2018-04-10T12:53:18Z

cmd/utils.go

+		if len(appUrl) != 0 {
+			fmt.Println("This component is externally exposed, and the URL will be removed")
+		}
+		appStore, err := storage.List(client, &occlient.VolumeConfig{


#295 changes storage.List, should we wait for that PR?
Or changing this will be handled in #295

#295 is already big PR, so it might make sense to wait for it.
What do you think @surajnarwade @containscafeine ?

kadel · 2018-04-10T13:44:53Z

currently this is blocked on #295

concaf · 2018-04-10T14:18:34Z

blocked on #287

kadel · 2018-04-19T11:57:50Z

cmd/application.go

+		// Project
+		currentProject := project.GetCurrent(client)
+		// Print App Information which will be deleted
+		err := printDeleteAppInfo(client, appName, currentProject)


Shouldn't this reuse functions from #287?

Text content is different, so I created separate function for this

To get component information, I am reusing the function,
https://github.com/redhat-developer/odo/pull/278/files#diff-b4695c7b6902e27273329af6261b0c73R19

concaf · 2018-04-19T14:10:50Z

@surajnarwade how about -

Deleting the component 'nodejs' will delete the following resources:
<print component info>
Do you want to delete? (y/N)

kadel · 2018-04-20T08:08:31Z

cmd/utils.go

+		fmt.Println("Component", cmpnt.Name, "will be deleted:")
+
+		if len(componentURL) != 0 {
+			fmt.Println("This component is externally exposed, and the URL will be removed")


can we add two spaces to the beginning of the line to indicate that this is related to the component mentioned above?

kadel · 2018-04-20T08:08:36Z

cmd/utils.go

+		}
+
+		for _, store := range appStore {
+			fmt.Println("This Component uses storage", store.Name, "of size", store.Size, "will be removed")


can we add two spaces to the beginning of the line to indicate that this is related to the component mentioned above?

kadel · 2018-04-20T08:10:12Z

cmd/utils.go

+		if err != nil {
+			return errors.Wrap(err, "unable to get component description")
+		}
+		fmt.Println("Component", cmpnt.Name, "will be deleted:")


The ":" looks strange when the component doesn't have any URLs or storages.

▶ odo app delete app Component backend will be deleted: Component frontend will be deleted: This component is externally exposed, and the URL will be removed Are you sure you want to delete the application: app? [y/N]

This looks better:

▶ odo app delete app Component backend will be deleted. Component frontend will be deleted. This component is externally exposed, and the URL will be removed Are you sure you want to delete the application: app? [y/N]

Fixes redhat-developer#204

kadel · 2018-04-23T07:06:53Z

ping @containscafeine

surajnarwade changed the title ~~Application Deletion will show what will be deleted~~ [WIP] Application Deletion will show what will be deleted Mar 27, 2018

surajnarwade force-pushed the app-del branch 2 times, most recently from d14ea39 to c32b030 Compare March 28, 2018 09:11

surajnarwade changed the title ~~[WIP] Application Deletion will show what will be deleted~~ Application Deletion will show what will be deleted Mar 28, 2018

surajnarwade force-pushed the app-del branch from c32b030 to 2df020e Compare March 28, 2018 12:01

surajnarwade requested a review from concaf March 28, 2018 13:16

surajnarwade force-pushed the app-del branch from 2df020e to fc927a4 Compare March 29, 2018 06:34

surajnarwade force-pushed the app-del branch from fc927a4 to 40425ab Compare March 29, 2018 09:40

concaf mentioned this pull request Mar 29, 2018

Make storage list as list output #285

Closed

kadel reviewed Mar 29, 2018

View reviewed changes

surajnarwade force-pushed the app-del branch from 40425ab to 3814396 Compare March 29, 2018 13:33

kadel approved these changes Apr 3, 2018

View reviewed changes

kadel reviewed Apr 3, 2018

View reviewed changes

concaf reviewed Apr 5, 2018

View reviewed changes

concaf suggested changes Apr 5, 2018

View reviewed changes

kadel requested changes Apr 5, 2018

View reviewed changes

surajnarwade force-pushed the app-del branch 2 times, most recently from 2899b71 to bc611b1 Compare April 9, 2018 12:39

surajnarwade force-pushed the app-del branch from bc611b1 to 9f4e903 Compare April 10, 2018 12:08

kadel reviewed Apr 10, 2018

View reviewed changes

kadel added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 10, 2018

kadel added the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Apr 10, 2018

surajnarwade force-pushed the app-del branch 2 times, most recently from 6736e56 to 133df8d Compare April 17, 2018 13:51

surajnarwade removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) labels Apr 17, 2018

surajnarwade force-pushed the app-del branch from 133df8d to a95910d Compare April 19, 2018 10:12

kadel reviewed Apr 19, 2018

View reviewed changes

surajnarwade force-pushed the app-del branch from a95910d to 2c8d3cc Compare April 19, 2018 14:06

kadel approved these changes Apr 20, 2018

View reviewed changes

surajnarwade force-pushed the app-del branch 2 times, most recently from fc69ace to dfb0b7d Compare April 20, 2018 13:13

Application Deletion will show what will be deleted

1842cca

Fixes redhat-developer#204

surajnarwade force-pushed the app-del branch from dfb0b7d to 1842cca Compare April 23, 2018 06:47

kadel approved these changes Apr 23, 2018

View reviewed changes

concaf approved these changes Apr 23, 2018

View reviewed changes

concaf merged commit 8f7073c into redhat-developer:master Apr 23, 2018

Application Deletion will show what will be deleted #278

Application Deletion will show what will be deleted #278

Uh oh!

Conversation

surajnarwade commented Mar 27, 2018

Uh oh!

surajnarwade commented Mar 28, 2018

Uh oh!

concaf commented Mar 28, 2018

Uh oh!

concaf commented Mar 28, 2018

Uh oh!

surajnarwade commented Mar 29, 2018

Uh oh!

surajnarwade commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajnarwade commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadel Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadel Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajnarwade commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadel commented Apr 10, 2018

Uh oh!

concaf commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

concaf commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadel commented Apr 23, 2018

Uh oh!

Uh oh!

surajnarwade commented Mar 29, 2018 •

edited

Loading

kadel Apr 5, 2018 •

edited

Loading

kadel Apr 5, 2018 •

edited

Loading