feat: initial commit for abra app volume ls/rm #51

2021-08-12T11:13:09Z

knoflook commented

2021-08-12 11:13:09 +00:00

No description provided.

~~roxxers referenced this pull request 2021-08-12 12:38:49 +00:00~~

App name vs domain is confusing #52

roxxers requested changes 2021-08-12 13:20:20 +00:00

roxxers left a comment

First-time contributor

Just needs to check for if the app exists to work great out of the box, i gave you a code snippet to fix that.

cli/app/volume.go Outdated

						
				@@ -0,0 +21,4 @@

					Action: func(c *cli.Context) error {

						appName := c.Args().First()

						if appName == "" {

							internal.ShowSubcommandHelpAndError(c, errors.New("No app name provided!"))

roxxers commented

2021-08-12 12:16:23 +00:00

First-time contributor

Style: Errors should not start with capital. https://staticcheck.io/docs/checks#ST1005

roxxers commented

2021-08-12 13:20:18 +00:00

First-time contributor

I am also unsure of using this error vs just printing the help page without the error. Since not giving the first arg seems to do so without error for other commands. This is more a thing for future clean up tbf and me rambling.

roxxers marked this conversation as resolved

cli/app/volume.go Outdated

						
				@@ -0,0 +27,4 @@

						if err != nil {

							logrus.Fatal(err)

						}

						host := appFiles[appName].Server

roxxers commented

2021-08-12 13:04:31 +00:00

First-time contributor

As discussed with my panic hour, this just isn't checking if the app itself exists leading to a weird error that is very confusing. Like "Context '' does not exist."

var host string
if app, ok := appFiles[appName]; ok {
	host = app.Server
}
if host == "" {
	logrus.Fatalf(`app "%s" does not exist`, appName)
}

Just tested this and it works great with this addition.

As discussed with my panic hour, this just isn't checking if the app itself exists leading to a weird error that is very confusing. Like "Context '' does not exist." ```go var host string if app, ok := appFiles[appName]; ok { host = app.Server } if host == "" { logrus.Fatalf(`app "%s" does not exist`, appName) } ``` Just tested this and it works great with this addition.

roxxers commented

2021-08-13 12:57:14 +00:00

First-time contributor

Just realised this should be simplified to

var host string
if app, ok := appFiles[appName]; ok {
	host = app.Server
} else {
	logrus.Fatalf(`app "%s" does not exist`, appName)
}

Just realised this should be simplified to ```go var host string if app, ok := appFiles[appName]; ok { host = app.Server } else { logrus.Fatalf(`app "%s" does not exist`, appName) } ```

knoflook marked this conversation as resolved

cli/app/volume.go Outdated

						
				@@ -0,0 +42,4 @@

						if err != nil {

							logrus.Fatal(err)

						}

						fmt.Println("DRIVER\t\t\tVOLUME NAME")

roxxers commented

2021-08-12 12:06:36 +00:00

First-time contributor

Minimal: We use table creation in the formatter for the abra/cli package

knoflook marked this conversation as resolved

cli/app/volume.go Outdated

						
				@@ -0,0 +66,4 @@

						if err != nil {

							logrus.Fatal(err)

						}

						host := appFiles[appName].Server

roxxers commented

2021-08-12 13:12:03 +00:00

First-time contributor

Same issue as before. Since you are doing this twice you could refactor some of the calls here into their own functions so both commands can use them?

knoflook marked this conversation as resolved

cli/app/volume.go Outdated

						
				@@ -0,0 +86,4 @@

							volumeNames = append(volumeNames, volume.Name)

						}

						var volumesToRemove []string

						if !internal.Force {