Add abra app remove command #43

Closed
knoflook wants to merge 7 commits from knoflook:dev into main
2 changed files with 62 additions and 72 deletions
Showing only changes of commit 09a06997e1 - Show all commits

View File

@ -3,6 +3,7 @@ package app
import ( import (
"context" "context"
"errors" "errors"
"fmt"
"os" "os"
"strings" "strings"
@ -26,133 +27,113 @@ var VolumesFlag = &cli.BoolFlag{
Destination: &Volumes, Destination: &Volumes,
} }
var Force bool
var ForceFlag = &cli.BoolFlag{
Name: "force",
Value: false,
Destination: &Force,
}
var appRemoveCommand = &cli.Command{ var appRemoveCommand = &cli.Command{
Name: "remove", Name: "remove",
knoflook marked this conversation as resolved Outdated

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)?
Aliases: []string{"rm", "delete"}, Aliases: []string{"rm", "delete"},
Flags: []cli.Flag{ Flags: []cli.Flag{
VolumesFlag, VolumesFlag,
ForceFlag, internal.ForceFlag,
}, },
Action: func(c *cli.Context) error { Action: func(c *cli.Context) error {
// Check if app name was provided by user appName := c.Args().First()
AppName := c.Args().First() if appName == "" {
if AppName == "" {
internal.ShowSubcommandHelpAndError(c, errors.New("No app name provided!")) internal.ShowSubcommandHelpAndError(c, errors.New("No app name provided!"))
} }
// Make sure that the user really wants to delete the app if !internal.Force {
if !Force {
response := false response := false
prompt := &survey.Confirm{ prompt := &survey.Confirm{
Message: "About to delete " + AppName + ", are you sure?", Message: fmt.Sprintf("About to delete %s, are you sure", appName),
knoflook marked this conversation as resolved Outdated

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 Outdated

appName (e.g. host)

`appName` (e.g. [host](https://git.coopcloud.tech/coop-cloud/go-abra/src/branch/main/cli/recipe/recipe.go#L189))
survey.AskOne(prompt, &response) survey.AskOne(prompt, &response)
if response == false { if !response {
return errors.New("User aborted app removal") return errors.New("User aborted app removal")
} }
} }
// Get app list appFiles, err := config.LoadAppFiles("")
AppFiles, err := config.LoadAppFiles("")
if err != nil { if err != nil {
return err logrus.Fatal(err)
knoflook marked this conversation as resolved Outdated

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

`Message: fmt.Sprintf("About to delete %s, are you sure?", appName)`
} }
AppPath := AppFiles[AppName].Path appPath := appFiles[appName].Path
host := AppFiles[AppName].Server fmt.Println(appFiles)
knoflook marked this conversation as resolved Outdated
if !response {
  logrus.Fatal(errors.New("User aborted app removal"))
}
```golang if !response { logrus.Fatal(errors.New("User aborted app removal")) } ```
// Initialize docker client host := appFiles[appName].Server
ctx := context.Background() ctx := context.Background()
cl, err := client.NewClientWithContext(host) cl, err := client.NewClientWithContext(host)
if err != nil { if err != nil {
return err logrus.Fatal(err)
decentral1se marked this conversation as resolved Outdated

appFiles, err := config.LoadAppFiles()

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

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[]
} }
// Remove the file err = os.Remove(appPath)
knoflook marked this conversation as resolved Outdated

logrus.Fatal(err)

`logrus.Fatal(err)`
err = os.Remove(AppPath)
if err != nil { if err != nil {
return err logrus.Fatal(err)
knoflook marked this conversation as resolved Outdated

appPath := AppFiles[appName].Path

`appPath := AppFiles[appName].Path`
} else { } else {
logrus.Info("File :" + AppPath + " removed.") logrus.Info(fmt.Sprintf("File: %s removed", appPath))
} }
// Check if the app has secrets
fs := filters.NewArgs() fs := filters.NewArgs()
fs.Add("name", AppName) fs.Add("name", appName)
SecretList, err := cl.SecretList(ctx, types.SecretListOptions{Filters: fs}) secretList, err := cl.SecretList(ctx, types.SecretListOptions{Filters: fs})
if err != nil { if err != nil {
return err logrus.Fatal(err)
} }
knoflook marked this conversation as resolved
Review
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)) ```
Secrets := make(map[string]string) secrets := make(map[string]string)
SecretNames := []string{} secretNames := []string{}
for _, cont := range SecretList { for _, cont := range secretList {
Secrets[cont.Spec.Annotations.Name] = cont.ID //we have to map the names to ID's secrets[cont.Spec.Annotations.Name] = cont.ID //we have to map the names to ID's
SecretNames = append(SecretNames, cont.Spec.Annotations.Name) secretNames = append(secretNames, cont.Spec.Annotations.Name)
} }
SecretNamesToRemove := []string{} secretNamesToRemove := []string{}
// Ask if the user really wants to remove the apps if internal.Force {
if Force { secretNamesToRemove = secretNames
SecretNamesToRemove = SecretNames
} else { } else {
knoflook marked this conversation as resolved Outdated

logrus.Fatal(err)

`logrus.Fatal(err)`
SecretsPrompt := &survey.MultiSelect{ secretsPrompt := &survey.MultiSelect{
Message: "Which secrets do you want to remove?", Message: "Which secrets do you want to remove?",
Options: SecretNames, Options: secretNames,
Default: SecretNames, Default: secretNames,
} }
survey.AskOne(SecretsPrompt, &SecretNamesToRemove) survey.AskOne(secretsPrompt, &secretNamesToRemove)
} }
// Actually remove the secrets for _, name := range secretNamesToRemove {
for _, name := range SecretNamesToRemove { err := cl.SecretRemove(ctx, secrets[name])
// DEBUG: SecretIDsToRemove = append(SecretIDsToRemove, Secrets[name])
err := cl.SecretRemove(ctx, Secrets[name])
if err != nil { if err != nil {
return err logrus.Fatal(err)
} else { } else {
logrus.Info("Secret: " + name + " removed") logrus.Info(fmt.Sprintf("Secret: %s removed", name))
} }
} }
// Get volumes associated with the app volumeListOKBody, err := cl.VolumeList(ctx, fs)
VolumeListOKBody, err := cl.VolumeList(ctx, fs) volumeList := volumeListOKBody.Volumes
VolumeList := VolumeListOKBody.Volumes
if err != nil { if err != nil {
return err logrus.Fatal(err)
} }
Vols := []string{} vols := []string{}
for _, vol := range VolumeList { for _, vol := range volumeList {
knoflook marked this conversation as resolved Outdated

Let's remove that 🙃

Let's remove that 🙃
Vols = append(Vols, vol.Name) vols = append(vols, vol.Name)
} }
knoflook marked this conversation as resolved Outdated
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)) ```
// Remove the volumes if desired
if Volumes == true {
RemoveVols := []string{} if Volumes {
// If there's no --force, ask if the user wants to remove removeVols := []string{}
if Force { if internal.Force {
RemoveVols = Vols removeVols = vols
} else { } else {
VolumesPrompt := &survey.MultiSelect{ volumesPrompt := &survey.MultiSelect{
Message: "Which volumes do you want to remove?", Message: "Which volumes do you want to remove?",
Options: Vols, Options: vols,
Default: Vols, Default: vols,
} }
knoflook marked this conversation as resolved
Review

logrus.Fatal(err)

`logrus.Fatal(err)`
survey.AskOne(VolumesPrompt, &RemoveVols) survey.AskOne(volumesPrompt, &removeVols)
} }
// Remove the volumes for _, vol := range removeVols {
for _, vol := range RemoveVols { err := cl.VolumeRemove(ctx, vol, internal.Force) // last argument is for force removing
err := cl.VolumeRemove(ctx, vol, Force) // false is for force removing
if err != nil { if err != nil {
return err logrus.Fatal(err)
} else { } else {
knoflook marked this conversation as resolved Outdated

if Volumes {

`if Volumes {`
logrus.Info("Volume " + vol + " removed") logrus.Info("Volume " + vol + " removed")
} }
} }
} else { } else {
logrus.Info("No volumes were removed. Volumes left: " + strings.Join(Vols, ", ")) logrus.Info("No volumes were removed. Volumes left: " + strings.Join(vols, ", "))
} }
return nil return nil

View File

@ -42,3 +42,12 @@ var ContextFlag = &cli.StringFlag{
Aliases: []string{"c"}, Aliases: []string{"c"},
Destination: &Context, Destination: &Context,
} }
var Force bool
var ForceFlag = &cli.BoolFlag{
Name: "force",
Value: false,
Aliases: []string{"f"},
Destination: &Force,
}