From 2f0b06cd219c9a2b31be3e25fc129e4d0388a245 Mon Sep 17 00:00:00 2001 From: windvalley Date: Fri, 14 Jan 2022 16:47:13 +0800 Subject: [PATCH] refactor: optimize error messages caused by improper use --- internal/cmd/command.go | 2 ++ internal/cmd/fetch.go | 2 ++ internal/cmd/push.go | 13 +++++++++ internal/cmd/script.go | 2 ++ internal/cmd/vault/decrypt.go | 6 ++-- internal/cmd/vault/encrypt.go | 7 +++-- internal/pkg/sshtask/sshtask.go | 52 ++++++++++++++++++--------------- pkg/batchssh/batchssh.go | 8 ++--- pkg/util/cobra.go | 13 +++++++++ pkg/util/error.go | 7 +++++ 10 files changed, 76 insertions(+), 36 deletions(-) diff --git a/internal/cmd/command.go b/internal/cmd/command.go index 3929ae8..e476731 100644 --- a/internal/cmd/command.go +++ b/internal/cmd/command.go @@ -90,6 +90,8 @@ Execute commands on target hosts.`, task.SetCommand(shellCommand) task.Start() + + util.CobraCheckErrWithHelp(cmd, task.CheckErr()) }, } diff --git a/internal/cmd/fetch.go b/internal/cmd/fetch.go index e6c77c5..698f3df 100644 --- a/internal/cmd/fetch.go +++ b/internal/cmd/fetch.go @@ -79,6 +79,8 @@ Copy files/dirs from target hosts to local.`, task.SetFetchOptions(localDstDir, tmpDir) task.Start() + + util.CobraCheckErrWithHelp(cmd, task.CheckErr()) }, } diff --git a/internal/cmd/push.go b/internal/cmd/push.go index d728596..67b47ef 100644 --- a/internal/cmd/push.go +++ b/internal/cmd/push.go @@ -115,6 +115,8 @@ Copy local files/dirs to target hosts.`, task.SetPushOptions(fileDstPath, allowOverwrite) task.Start() + + util.CobraCheckErrWithHelp(cmd, task.CheckErr()) }, } @@ -134,4 +136,15 @@ func init() { false, "allow overwrite files/dirs if they already exist on target hosts", ) + + pushCmd.SetHelpFunc(func(command *cobra.Command, strings []string) { + util.CobraMarkHiddenGlobalFlags( + command, + "run.sudo", + "run.as-user", + "run.lang", + ) + + command.Parent().HelpFunc()(command, strings) + }) } diff --git a/internal/cmd/script.go b/internal/cmd/script.go index 1527d6d..8f82d2a 100644 --- a/internal/cmd/script.go +++ b/internal/cmd/script.go @@ -85,6 +85,8 @@ Execute a local shell script on target hosts.`, task.SetScriptOptions(destPath, remove, force) task.Start() + + util.CobraCheckErrWithHelp(cmd, task.CheckErr()) }, } diff --git a/internal/cmd/vault/decrypt.go b/internal/cmd/vault/decrypt.go index 7ad08c6..e97740c 100644 --- a/internal/cmd/vault/decrypt.go +++ b/internal/cmd/vault/decrypt.go @@ -45,11 +45,11 @@ Decrypt content encrypted by vault.`, $ gossh vault decrypt GOSSH-AES256:a5c1b3c0cdad4669f84 -V /path/vault-password-file`, Args: func(cmd *cobra.Command, args []string) error { if len(args) < 1 { - util.CheckErr("requires one arg to represent the vault encrypted content") + util.CobraCheckErrWithHelp(cmd, "requires one arg to represent the vault encrypted content") } if len(args) > 1 { - util.CheckErr("to many args, only need one") + util.CobraCheckErrWithHelp(cmd, "to many args, only need one") } if !aes.IsAES256CipherText(args[0]) { @@ -66,6 +66,6 @@ Decrypt content encrypted by vault.`, } util.CheckErr(err) - fmt.Println(plainText) + fmt.Printf("\n%s\n", plainText) }, } diff --git a/internal/cmd/vault/encrypt.go b/internal/cmd/vault/encrypt.go index 2c6ab69..475b44d 100644 --- a/internal/cmd/vault/encrypt.go +++ b/internal/cmd/vault/encrypt.go @@ -45,23 +45,24 @@ Encrypt sensitive content.`, $ gossh vault encrypt your-sensitive-content -V /path/vault-password-file`, Args: func(cmd *cobra.Command, args []string) error { if len(args) < 1 { - util.CheckErr("requires one arg to represent the plaintxt to be encrypted") + util.CobraCheckErrWithHelp(cmd, "requires one arg to represent the plaintxt to be encrypted") } if len(args) > 1 { - util.CheckErr("to many args, only need one") + util.CobraCheckErrWithHelp(cmd, "to many args, only need one") } return nil }, Run: func(cmd *cobra.Command, args []string) { vaultPass := getVaultConfirmPassword() + encryptContent, err := aes.AES256Encode(args[0], vaultPass) if err != nil { err = fmt.Errorf("encrypt failed: %w", err) } util.CheckErr(err) - fmt.Println(encryptContent) + fmt.Printf("\n%s\n", encryptContent) }, } diff --git a/internal/pkg/sshtask/sshtask.go b/internal/pkg/sshtask/sshtask.go index 38832a8..d0834e5 100644 --- a/internal/pkg/sshtask/sshtask.go +++ b/internal/pkg/sshtask/sshtask.go @@ -111,6 +111,8 @@ type Task struct { taskOutput chan taskResult detailOutput chan detailResult + + err error } // NewTask ... @@ -227,7 +229,8 @@ func (t *Task) BatchRun() { allHosts, err := t.getAllHosts() if err != nil { - util.CheckErr(err) + t.err = err + return } log.Debugf("got target hosts, count: %d", len(allHosts)) @@ -251,29 +254,31 @@ func (t *Task) BatchRun() { switch t.taskType { case CommandTask: if t.command == "" { - util.CheckErr(errors.New("need flag '-e/--execute' or '-L/--hosts.list'")) + t.err = errors.New("need flag '-e/--execute' or '-L/--hosts.list'") } case ScriptTask: if t.scriptFile == "" { - util.CheckErr(errors.New("need flag '-e/--execute' or '-L/--hosts.list'")) + t.err = errors.New("need flag '-e/--execute' or '-L/--hosts.list'") } case PushTask: if t.pushFiles == nil || len(t.pushFiles.files) == 0 { - util.CheckErr(errors.New("need flag '-f/--files' or '-L/--hosts.list'")) + t.err = errors.New("need flag '-f/--files' or '-L/--hosts.list'") } case FetchTask: if len(t.fetchFiles) == 0 { - util.CheckErr(errors.New("need flag '-f/--files' or '-L/--hosts.list'")) - } - - if len(t.dstDir) == 0 { - util.CheckErr(errors.New("need flag '-d/--dest-path' or '-L/--hosts.list'")) + t.err = errors.New("need flag '-f/--files' or '-L/--hosts.list'") + } else if len(t.dstDir) == 0 { + t.err = errors.New("need flag '-d/--dest-path' or '-L/--hosts.list'") + } else { + if !util.DirExists(t.dstDir) { + err := os.MkdirAll(t.dstDir, os.ModePerm) + util.CheckErr(err) + } } + } - if !util.DirExists(t.dstDir) { - err := os.MkdirAll(t.dstDir, os.ModePerm) - util.CheckErr(err) - } + if t.err != nil { + return } t.buildSSHClient() @@ -348,6 +353,11 @@ func (t *Task) HandleOutput() { } } +// CheckErr ... +func (t *Task) CheckErr() error { + return t.err +} + func (t *Task) getAllHosts() ([]string, error) { var hosts []string @@ -392,17 +402,15 @@ func (t *Task) getAllHosts() ([]string, error) { } if len(hosts) == 0 { - return nil, fmt.Errorf("no target hosts provided") + return nil, fmt.Errorf("need target hosts, you can specify hosts file by flag '-H' or " + + "provide host/pattern as positional arguments") } return util.RemoveDuplStr(hosts), nil } func (t *Task) buildSSHClient() { - var ( - sshClient *batchssh.Client - err error - ) + var sshClient *batchssh.Client password, err := t.getPassword() if err != nil { @@ -414,7 +422,7 @@ func (t *Task) buildSSHClient() { if t.configFlags.Proxy.Server != "" { proxyAuths := t.getProxySSHAuthMethods(&password) - sshClient, err = batchssh.NewClient( + sshClient = batchssh.NewClient( t.configFlags.Auth.User, password, auths, @@ -430,7 +438,7 @@ func (t *Task) buildSSHClient() { ), ) } else { - sshClient, err = batchssh.NewClient( + sshClient = batchssh.NewClient( t.configFlags.Auth.User, password, auths, @@ -441,10 +449,6 @@ func (t *Task) buildSSHClient() { ) } - if err != nil { - util.CheckErr(err) - } - t.sshClient = sshClient } diff --git a/pkg/batchssh/batchssh.go b/pkg/batchssh/batchssh.go index 453f61b..a051788 100644 --- a/pkg/batchssh/batchssh.go +++ b/pkg/batchssh/batchssh.go @@ -83,11 +83,7 @@ type Proxy struct { } // NewClient session. -func NewClient( - user, password string, - auths []ssh.AuthMethod, - options ...func(*Client), -) (*Client, error) { +func NewClient(user, password string, auths []ssh.AuthMethod, options ...func(*Client)) *Client { client := Client{ User: user, Password: password, @@ -103,7 +99,7 @@ func NewClient( option(&client) } - return &client, nil + return &client } // BatchRun command on remote servers. diff --git a/pkg/util/cobra.go b/pkg/util/cobra.go index 474dcd5..185de48 100644 --- a/pkg/util/cobra.go +++ b/pkg/util/cobra.go @@ -7,6 +7,19 @@ import ( "github.com/spf13/pflag" ) +// CobraCheckErrWithHelp instead of cobra default behavior. +func CobraCheckErrWithHelp(cmd *cobra.Command, errMsg interface{}) { + if errMsg != nil { + PrintErr(errMsg) + + _ = cmd.Help() + + fmt.Println() + + CheckErr(errMsg) + } +} + // CobraMarkHiddenGlobalFlags that from params. func CobraMarkHiddenGlobalFlags(command *cobra.Command, flags ...string) { for _, v := range flags { diff --git a/pkg/util/error.go b/pkg/util/error.go index 05c38d3..0ad10ec 100644 --- a/pkg/util/error.go +++ b/pkg/util/error.go @@ -36,3 +36,10 @@ func CheckErr(msg interface{}) { os.Exit(1) } } + +// PrintErr with red color if err not nil. +func PrintErr(msg interface{}) { + if msg != nil { + fmt.Fprintln(os.Stderr, color.RedString("Error:"), msg) + } +}