coop-cloud/abra
coop-cloud
/
abra
5
2
Fork 14
Code Issues Pull Requests 4 Packages Releases 36 Activity

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

Merged
roxxers merged 2 commits from knoflook/abra:secret-create into main 2021-08-18 18:29:14 +00:00
Conversation 2 Commits 2 Files Changed 3 +165
knoflook commented 2021-08-12 11:13:09 +00:00
Owner
Copy Link
No description provided.
roxxers requested changes 2021-08-12 13:20:20 +00:00
roxxers left a comment
First-time contributor
Copy Link

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.

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
Copy Link

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

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
Copy Link

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.

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
Copy Link

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
Copy Link

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
Copy Link

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

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
Copy Link

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?

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 {
roxxers commented 2021-08-12 13:17:51 +00:00
First-time contributor
Copy Link

This is great!!

This is great!!
roxxers marked this conversation as resolved
knoflook force-pushed secret-create from bfa2237103 to c3088a5158 2021-08-17 18:40:42 +00:00 Compare
knoflook commented 2021-08-18 06:55:28 +00:00
Author
Owner
Copy Link

@roxxers could you review the code? Should be good now!

@roxxers could you review the code? Should be good now!
roxxers merged commit d2d0ce3d05 into main 2021-08-18 18:29:14 +00:00
roxxers commented 2021-08-18 18:29:22 +00:00
First-time contributor
Copy Link

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No reviewers
roxxers
Labels
Clear labels   
bug

Something is not working

  
build

go build related issues

  
ci/cd

Building things with CI/CD

  
contributing

Dealing with contributing

  
design

UI/UX

  
documentation

Documenting all the things

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
meta

relating to the project or repo itself

  
question

More information is needed

  
security

Security related

  
wontfix

This won't be fixed

No Label
bug
build
ci/cd
contributing
design
documentation
duplicate
enhancement
help wanted
invalid
meta
question
security
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: coop-cloud/abra#51
No description provided.
Delete Branch "knoflook/abra:secret-create"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?