feat: app rm - check if the app is undeployed before removing #61

2021-09-03T13:07:23Z

knoflook commented

2021-09-03 13:07:23 +00:00

Also fixes app rm not checking for the volumes and secrets at all.

roxxers reviewed 2021-09-03 15:05:19 +00:00

cli/app/remove.go Outdated

						
				@@ -68,0 +68,4 @@

						if !internal.Force {

							// get app status and check if it's deployed

							statuses := map[string]string{}

							statuses, err = config.GetAppStatuses(appFiles)

roxxers commented

2021-09-03 15:05:14 +00:00

First-time contributor

Is there a reason for doing it like this vs

statuses, err := config.GetAppStatuses(appFiles)

Is there a reason for doing it like this vs `statuses, err := config.GetAppStatuses(appFiles)`

knoflook commented

2021-09-04 05:53:16 +00:00

nope, I just brainlessly copied it from cli/app/list.go lol

roxxers commented

2021-09-03 15:12:01 +00:00

First-time contributor

Everything looks fine but did comment on one line of the code that looks like it can be simplifed but I didn't write it so there could be a reason for that you encountered. I can't test it right now though so I'll have to ping @decentral1se to test the code when he can. I don't see why not atm though as the code looks good.

knoflook force-pushed dev from 64608a5ab0 to 0d548869e8

2021-09-04 06:02:15 +00:00

Compare

decentral1se requested changes 2021-09-04 18:25:43 +00:00

cli/app/remove.go

						
				@@ -68,0 +72,4 @@

								logrus.Fatal(err)

							}

							if statuses[appName] == "deployed" {

								return errors.New("app still deployed. Run abra app undeploy first. (or pass --force)")

decentral1se commented

2021-09-04 18:25:34 +00:00

logrus.Fatal(...)

```golang logrus.Fatal(...) ```

roxxers commented

2021-09-06 20:16:47 +00:00

First-time contributor

logrus.Fatal(...)

Unsure if we want to move to the return flo uses if urfave/cli like actually outputs it to stderr. I used logrus cause its there and we will use it for debug, warn, and info. But for errors we could return to avoid the messy error output. Like the way it looks is fine for a daemon but im reconsidering that for a cli program

> ```golang > logrus.Fatal(...) > ``` Unsure if we want to move to the return flo uses if urfave/cli like actually outputs it to stderr. I used logrus cause its there and we will use it for debug, warn, and info. But for errors we could return to avoid the messy error output. Like the way it looks is fine for a daemon but im reconsidering that for a cli program

decentral1se referenced this issue from a commit

2021-09-05 19:55:42 +00:00

fix: check for deployment of app before removing

decentral1se commented

2021-09-05 19:56:47 +00:00

Merged in cc249e8187066cb9ce7a15e6ce97e843c953a135! Thanks!

I like the approach of using config.GetAppStatuses but I realise now that it looks up all statuses of all apps instead of just one app. We'll need to figure out a separate API for that.

Merged in cc249e8187066cb9ce7a15e6ce97e843c953a135! Thanks! I like the approach of using `config.GetAppStatuses` but I realise now that it looks up all statuses of all apps instead of just one app. We'll need to figure out a separate API for that.

decentral1se closed this pull request

2021-09-05 19:56:57 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: toolshed/abra#61

Allow edits from maintainers