Add abra app remove command #43

Closed
knoflook wants to merge 7 commits from knoflook:dev into main
Owner

When ran with no flags, it removes the env file and lets you choose which secrets to remove. If you add --volumes, it also lets you choose which volumes should be removed.

If you only specify --force, the env file and all secrets will be removed
If you specify --force --volumes, the volumes will also be removed.

When ran with no flags, it removes the env file and lets you choose which secrets to remove. If you add --volumes, it also lets you choose which volumes should be removed. If you only specify --force, the env file and all secrets will be removed If you specify --force --volumes, the volumes will also be removed.
knoflook added 3 commits 2021-08-05 15:04:47 +00:00
decentral1se approved these changes 2021-08-05 15:50:09 +00:00
decentral1se left a comment
Owner

Nice one! Buncha comments, hope it is not overwhelming 🙈

If you only specify --force, the env file and all secrets will be removed

Can we have it remove env file, secrets and volumes?

Thanks 🚀

(hmu on matrix if anything is unclear)

Nice one! Buncha comments, hope it is not overwhelming 🙈 > If you only specify --force, the env file and all secrets will be removed Can we have it remove env file, secrets and volumes? Thanks 🚀 (hmu on matrix if anything is unclear)
@ -17,1 +28,4 @@
var Force bool
var ForceFlag = &cli.BoolFlag{
Owner

This is a general flag we'll add to other sub-commands, it can live in cli/internal/common.go?

This is a general flag we'll add to other sub-commands, it can live in [cli/internal/common.go](https://git.coopcloud.tech/coop-cloud/go-abra/src/branch/main/cli/internal/common.go)?
knoflook marked this conversation as resolved
@ -23,0 +42,4 @@
ForceFlag,
},
Action: func(c *cli.Context) error {
// Check if app name was provided by user
Owner

We haven't discussed this but the existing convention that I see is that we don't add comments to code to explain what it is doing unless it is doing something strange or difficult. Otherwise, we comment to explain why we're doing something (context, not what it is doing).

The issue is, that when the code changes, you might forget to change the comment and then the next person might be caught reading the comment which describes code that doesn'te exist anymore. I would say have a look around the rest of the codebase at comment usage and perhaps remove the obvious ones in this function.

For example the // we have to map the names to ID's comment is a good one, I'd keep that.

We haven't discussed this but the existing convention that I see is that we don't add comments to code to explain what it is doing unless it is doing something strange or difficult. Otherwise, we comment to explain why we're doing something (context, not what it is doing). The issue is, that when the code changes, you might forget to change the comment and then the next person might be caught reading the comment which describes code that doesn'te exist anymore. I would say have a look around the rest of the codebase at comment usage and perhaps remove the obvious ones in this function. For example the `// we have to map the names to ID's` comment is a good one, I'd keep that.
knoflook marked this conversation as resolved
@ -23,0 +43,4 @@
},
Action: func(c *cli.Context) error {
// Check if app name was provided by user
AppName := c.Args().First()
Owner

appName (e.g. host)

`appName` (e.g. [host](https://git.coopcloud.tech/coop-cloud/go-abra/src/branch/main/cli/recipe/recipe.go#L189))
knoflook marked this conversation as resolved
@ -23,0 +51,4 @@
if !Force {
response := false
prompt := &survey.Confirm{
Message: "About to delete " + AppName + ", are you sure?",
Owner

Message: fmt.Sprintf("About to delete %s, are you sure?", appName)

`Message: fmt.Sprintf("About to delete %s, are you sure?", appName)`
knoflook marked this conversation as resolved
@ -23,0 +54,4 @@
Message: "About to delete " + AppName + ", are you sure?",
}
survey.AskOne(prompt, &response)
if response == false {
Owner
if !response {
  logrus.Fatal(errors.New("User aborted app removal"))
}
```golang if !response { logrus.Fatal(errors.New("User aborted app removal")) } ```
knoflook marked this conversation as resolved
@ -23,0 +59,4 @@
}
}
// Get app list
AppFiles, err := config.LoadAppFiles("")
Owner

appFiles, err := config.LoadAppFiles()

`appFiles, err := config.LoadAppFiles()`
Author
Owner

Can't do it that way, if you remove "" LoadAppFiles returns an empty map[]

Can't do it that way, if you remove "" LoadAppFiles returns an empty map[]
decentral1se marked this conversation as resolved
@ -23,0 +61,4 @@
// Get app list
AppFiles, err := config.LoadAppFiles("")
if err != nil {
return err
Owner

logrus.Fatal(err)

`logrus.Fatal(err)`
knoflook marked this conversation as resolved
@ -23,0 +63,4 @@
if err != nil {
return err
}
AppPath := AppFiles[AppName].Path
Owner

appPath := AppFiles[appName].Path

`appPath := AppFiles[appName].Path`
knoflook marked this conversation as resolved
@ -23,0 +73,4 @@
}
// Remove the file
err = os.Remove(AppPath)
if err != nil {
Owner
if err != nil {
	logrus.Fatal(err)
}
logrus.Info(fmt.Sprintf("File: %s removed", appPath))
```golang if err != nil { logrus.Fatal(err) } logrus.Info(fmt.Sprintf("File: %s removed", appPath)) ```
knoflook marked this conversation as resolved
@ -23,0 +84,4 @@
fs.Add("name", AppName)
SecretList, err := cl.SecretList(ctx, types.SecretListOptions{Filters: fs})
if err != nil {
return err
Owner

logrus.Fatal(err)

`logrus.Fatal(err)`
knoflook marked this conversation as resolved
@ -23,0 +108,4 @@
// Actually remove the secrets
for _, name := range SecretNamesToRemove {
// DEBUG: SecretIDsToRemove = append(SecretIDsToRemove, Secrets[name])
Owner

Let's remove that 🙃

Let's remove that 🙃
knoflook marked this conversation as resolved
@ -23,0 +110,4 @@
for _, name := range SecretNamesToRemove {
// DEBUG: SecretIDsToRemove = append(SecretIDsToRemove, Secrets[name])
err := cl.SecretRemove(ctx, Secrets[name])
if err != nil {
Owner
if err != nil {
	logrus.Fatal(err)
}
logrus.Info(fmt.Sprintf("Secret: %s removed", name))
```golang if err != nil { logrus.Fatal(err) } logrus.Info(fmt.Sprintf("Secret: %s removed", name)) ```
knoflook marked this conversation as resolved
@ -23,0 +121,4 @@
VolumeListOKBody, err := cl.VolumeList(ctx, fs)
VolumeList := VolumeListOKBody.Volumes
if err != nil {
return err
Owner

logrus.Fatal(err)

`logrus.Fatal(err)`
knoflook marked this conversation as resolved
@ -23,0 +128,4 @@
Vols = append(Vols, vol.Name)
}
// Remove the volumes if desired
if Volumes == true {
Owner

if Volumes {

`if Volumes {`
knoflook marked this conversation as resolved
Owner

Lol accidentally clicked that approve button but anyway, you get the idea 🙈

Lol accidentally clicked that approve button but anyway, you get the idea 🙈
knoflook added 1 commit 2021-08-05 16:54:28 +00:00
knoflook added 3 commits 2021-08-05 16:59:50 +00:00
Author
Owner

In the image: discussion about --force behaviour without --volumes specified

In the image: discussion about `--force` behaviour without `--volumes` specified
Owner

Merged in 6732edf8db 🥳

(some git rebase-foo which we can co-work on figuring out next time)

Also took a pass in 36af302d5f to drop some "dangling else" stuff, down to chat about that also.

Thanks so much for this!

Merged in https://git.coopcloud.tech/coop-cloud/go-abra/commit/6732edf8dbef56d258a0310df9951a7306206fe3 🥳 (some `git rebase`-foo which we can co-work on figuring out next time) Also took a pass in https://git.coopcloud.tech/coop-cloud/go-abra/commit/36af302d5fc71e4e17dff6aed50920abc4a3465f to drop some "dangling else" stuff, down to chat about that also. Thanks so much for this!
decentral1se closed this pull request 2021-08-06 10:23:59 +00:00

Pull request closed

Sign in to join this conversation.
No description provided.