refactor(cli): improve error handling. Fixes #1935

It's tricky to write unit tests for the CLI code for three main reasons:
exiting on errors, lack of dependency injection, and insufficient mocks.
#3917 tried to solve all
of these issues, but appears to have been abandoned. I thought about
trying to revive that PR, but it's been over 4 years and there's
certainly going to be a ton of conflicts. I figure it's better to take a
more focused, incremental approach.

This PR is focused on improving error handling in the following ways:
1. Do argument validation using [cobra
   validators](https://cobra.dev/#positional-and-custom-arguments)
   instead of custom logic in the `Run` function. Besides being
   simpler, this leads to better error messages.
2. Replace all `Run` functions with `RunE`, which allows returning
   errors. This is the same approach that Argo Rollouts is taking
   (example:
   https://github.com/argoproj/argo-rollouts/blob/00e39b114849010dd96221a8db441d58e860d4d0/pkg/kubectl-argo-rollouts/cmd/abort/abort.go#L41)
3. Replace nearly all calls to `log.Fatal()`/`errors.CheckError()`/etc
   with error propagation.

Signed-off-by: Mason Malone <mmalone@adobe.com>

cmd/argo/commands/archive/delete.go

@@ -3,7 +3,6 @@ package archive
 import (
  "fmt"
 
- "github.com/argoproj/pkg/errors"
  "github.com/spf13/cobra"
 
  client "github.com/argoproj/argo-workflows/v3/cmd/argo/commands/client"
@@ -14,15 +13,22 @@ func NewDeleteCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "delete UID...",
  Short: "delete a workflow in the archive",
- Run: func(cmd *cobra.Command, args []string) {
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ RunE: func(cmd *cobra.Command, args []string) error {
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  for _, uid := range args {
- _, err = serviceClient.DeleteArchivedWorkflow(ctx, &workflowarchivepkg.DeleteArchivedWorkflowRequest{Uid: uid})
- errors.CheckError(err)
+ if _, err = serviceClient.DeleteArchivedWorkflow(ctx, &workflowarchivepkg.DeleteArchivedWorkflowRequest{Uid: uid}); err != nil {
+ return err
+ }
  fmt.Printf("Archived workflow '%s' deleted\n", uid)
  }
+ return nil
  },
  }
  return command

cmd/argo/commands/archive/get.go

@@ -4,9 +4,7 @@ import (
  "encoding/json"
  "fmt"
  "log"
- "os"
 
- "github.com/argoproj/pkg/errors"
  "github.com/argoproj/pkg/humanize"
  "github.com/spf13/cobra"
  "sigs.k8s.io/yaml"
@@ -21,19 +19,24 @@ func NewGetCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "get UID",
  Short: "get a workflow in the archive",
- Run: func(cmd *cobra.Command, args []string) {
- if len(args) != 1 {
- cmd.HelpFunc()(cmd, args)
- os.Exit(1)
- }
+ Args: cobra.ExactArgs(1),
+ RunE: func(cmd *cobra.Command, args []string) error {
  uid := args[0]
 
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  wf, err := serviceClient.GetArchivedWorkflow(ctx, &workflowarchivepkg.GetArchivedWorkflowRequest{Uid: uid})
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  printWorkflow(wf, output)
+ return nil
  },
  }
  command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide")

cmd/argo/commands/archive/list.go

@@ -5,7 +5,6 @@ import (
  "os"
  "sort"
 
- "github.com/argoproj/pkg/errors"
  log "github.com/sirupsen/logrus"
  "github.com/spf13/cobra"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -25,15 +24,21 @@ func NewListCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "list",
  Short: "list workflows in the archive",
- Run: func(cmd *cobra.Command, args []string) {
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ RunE: func(cmd *cobra.Command, args []string) error {
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  namespace := client.Namespace()
  workflows, err := listArchivedWorkflows(ctx, serviceClient, namespace, selector, chunkSize)
- errors.CheckError(err)
- err = printer.PrintWorkflows(workflows, os.Stdout, printer.PrintOpts{Output: output, Namespace: true, UID: true})
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
+ return printer.PrintWorkflows(workflows, os.Stdout, printer.PrintOpts{Output: output, Namespace: true, UID: true})
  },
  }
  command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide")

cmd/argo/commands/archive/list_label_keys.go

@@ -3,7 +3,6 @@ package archive
 import (
  "fmt"
 
- "github.com/argoproj/pkg/errors"
  "github.com/spf13/cobra"
 
  "github.com/argoproj/argo-workflows/v3/cmd/argo/commands/client"
@@ -14,15 +13,23 @@ func NewListLabelKeyCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "list-label-keys",
  Short: "list workflows label keys in the archive",
- Run: func(cmd *cobra.Command, args []string) {
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ RunE: func(cmd *cobra.Command, args []string) error {
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  keys, err := serviceClient.ListArchivedWorkflowLabelKeys(ctx, &workflowarchivepkg.ListArchivedWorkflowLabelKeysRequest{})
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  for _, str := range keys.Items {
  fmt.Printf("%s\n", str)
  }
+ return nil
  },
  }
  return command

cmd/argo/commands/archive/list_label_values.go

@@ -18,20 +18,28 @@ func NewListLabelValueCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "list-label-values",
  Short: "get workflow label values in the archive",
- Run: func(cmd *cobra.Command, args []string) {
+ RunE: func(cmd *cobra.Command, args []string) error {
  listOpts := &metav1.ListOptions{
  LabelSelector: selector,
  }
 
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  labels, err := serviceClient.ListArchivedWorkflowLabelValues(ctx, &workflowarchivepkg.ListArchivedWorkflowLabelValuesRequest{ListOptions: listOpts})
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
 
  for _, str := range labels.Items {
  fmt.Printf("%s\n", str)
  }
+ return nil
  },
  }
  command.Flags().StringVarP(&selector, "selector", "l", "", "Selector (label query) to query on, allows 1 value (e.g. -l key1)")

cmd/argo/commands/archive/resubmit.go

@@ -3,7 +3,6 @@ package archive
 import (
  "context"
 
- "github.com/argoproj/pkg/errors"
  "github.com/spf13/cobra"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/apimachinery/pkg/types"
@@ -67,18 +66,22 @@ func NewResubmitCommand() *cobra.Command {
 
  argo archive resubmit --log uid
 `,
- Run: func(cmd *cobra.Command, args []string) {
+ RunE: func(cmd *cobra.Command, args []string) error {
  if cmd.Flag("priority").Changed {
  cliSubmitOpts.Priority = &resubmitOpts.priority
  }
 
- ctx, apiClient := client.NewAPIClient(cmd.Context())
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
+ }
  serviceClient := apiClient.NewWorkflowServiceClient() // needed for wait watch or log flags
  archiveServiceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  resubmitOpts.namespace = client.Namespace()
- err = resubmitArchivedWorkflows(ctx, archiveServiceClient, serviceClient, resubmitOpts, cliSubmitOpts, args)
- errors.CheckError(err)
+ return resubmitArchivedWorkflows(ctx, archiveServiceClient, serviceClient, resubmitOpts, cliSubmitOpts, args)
  },
  }
 
@@ -142,7 +145,7 @@ func resubmitArchivedWorkflows(ctx context.Context, archiveServiceClient workflo
 
  if len(resubmittedUids) == 1 {
  // watch or wait when there is only one workflow retried
- common.WaitWatchOrLog(ctx, serviceClient, lastResubmitted.Namespace, []string{lastResubmitted.Name}, cliSubmitOpts)
+ return common.WaitWatchOrLog(ctx, serviceClient, lastResubmitted.Namespace, []string{lastResubmitted.Name}, cliSubmitOpts)
  }
  return nil
 }

cmd/argo/commands/archive/retry.go

@@ -2,10 +2,9 @@ package archive
 
 import (
  "context"
+ "errors"
  "fmt"
- "os"
 
- "github.com/argoproj/pkg/errors"
  "github.com/spf13/cobra"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/apimachinery/pkg/fields"
@@ -70,20 +69,25 @@ func NewRetryCommand() *cobra.Command {
 
  argo archive retry --log uid
 `,
- Run: func(cmd *cobra.Command, args []string) {
+ Args: func(cmd *cobra.Command, args []string) error {
  if len(args) == 0 && !retryOpts.hasSelector() {
- cmd.HelpFunc()(cmd, args)
- os.Exit(1)
+ return errors.New("requires either selector or workflow")
+ }
+ return nil
+ },
+ RunE: func(cmd *cobra.Command, args []string) error {
+ ctx, apiClient, err := client.NewAPIClient(cmd.Context())
+ if err != nil {
+ return err
  }
-
- ctx, apiClient := client.NewAPIClient(cmd.Context())
  serviceClient := apiClient.NewWorkflowServiceClient()
  archiveServiceClient, err := apiClient.NewArchivedWorkflowServiceClient()
- errors.CheckError(err)
+ if err != nil {
+ return err
+ }
  retryOpts.namespace = client.Namespace()
 
- err = retryArchivedWorkflows(ctx, archiveServiceClient, serviceClient, retryOpts, cliSubmitOpts, args)
- errors.CheckError(err)
+ return retryArchivedWorkflows(ctx, archiveServiceClient, serviceClient, retryOpts, cliSubmitOpts, args)
  },
  }
 
@@ -146,7 +150,7 @@ func retryArchivedWorkflows(ctx context.Context, archiveServiceClient workflowar
  }
  if len(retriedUids) == 1 {
  // watch or wait when there is only one workflow retried
- common.WaitWatchOrLog(ctx, serviceClient, lastRetried.Namespace, []string{lastRetried.Name}, cliSubmitOpts)
+ return common.WaitWatchOrLog(ctx, serviceClient, lastRetried.Namespace, []string{lastRetried.Name}, cliSubmitOpts)
  }
  return nil
 }

cmd/argo/commands/archive/root.go

@@ -8,8 +8,8 @@ func NewArchiveCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "archive",
  Short: "manage the workflow archive",
- Run: func(cmd *cobra.Command, args []string) {
- cmd.HelpFunc()(cmd, args)
+ RunE: func(cmd *cobra.Command, args []string) error {
+ return cmd.Help()
  },
  }
 

cmd/argo/commands/auth/root.go

@@ -8,8 +8,8 @@ func NewAuthCommand() *cobra.Command {
  command := &cobra.Command{
  Use: "auth",
  Short: "manage authentication settings",
- Run: func(cmd *cobra.Command, args []string) {
- cmd.HelpFunc()(cmd, args)
+ RunE: func(cmd *cobra.Command, args []string) error {
+ return cmd.Help()
  },
  }
  command.AddCommand(NewTokenCommand())

cmd/argo/commands/auth/token.go

@@ -2,7 +2,6 @@ package auth
 
 import (
  "fmt"
- "os"
 
  "github.com/spf13/cobra"
 
@@ -13,12 +12,14 @@ func NewTokenCommand() *cobra.Command {
  return &cobra.Command{
  Use: "token",
  Short: "Print the auth token",
- Run: func(cmd *cobra.Command, args []string) {
- if len(args) != 0 {
- cmd.HelpFunc()(cmd, args)
- os.Exit(1)
+ Args: cobra.NoArgs,
+ RunE: func(cmd *cobra.Command, args []string) error {
+ authString, err := client.GetAuthString()
+ if err != nil {
+ return err
  }
- fmt.Println(client.GetAuthString())
+ fmt.Println(authString)
+ return nil
  },
  }
 }

cmd/argo/commands/client/conn.go

@@ -54,23 +54,23 @@ func AddAPIClientFlagsToCmd(cmd *cobra.Command) {
  cmd.PersistentFlags().BoolVarP(&ArgoServerOpts.InsecureSkipVerify, "insecure-skip-verify", "k", os.Getenv("ARGO_INSECURE_SKIP_VERIFY") == "true", "If true, the Argo Server's certificate will not be checked for validity. This will make your HTTPS connections insecure. Defaults to the ARGO_INSECURE_SKIP_VERIFY environment variable.")
 }
 
-func NewAPIClient(ctx context.Context) (context.Context, apiclient.Client) {
- ctx, client, err := apiclient.NewClientFromOpts(
+func NewAPIClient(ctx context.Context) (context.Context, apiclient.Client, error) {
+ return apiclient.NewClientFromOpts(
  apiclient.Opts{
  ArgoServerOpts: ArgoServerOpts,
  InstanceID: instanceID,
  AuthSupplier: func() string {
- return GetAuthString()
+ authString, err := GetAuthString()
+ if err != nil {
+ log.Fatal(err)
+ }
+ return authString
  },
  ClientConfigSupplier: func() clientcmd.ClientConfig { return GetConfig() },
  Offline: Offline,
  OfflineFiles: OfflineFiles,
  Context: ctx,
  })
- if err != nil {
- log.Fatal(err)
- }
- return ctx, client
 }
 
 func Namespace() string {
@@ -91,20 +91,20 @@ func Namespace() string {
  return namespace
 }
 
-func GetAuthString() string {
+func GetAuthString() (string, error) {
  token, ok := os.LookupEnv("ARGO_TOKEN")
  if ok {
- return token
+ return token, nil
  }
  restConfig, err := GetConfig().ClientConfig()
  if err != nil {
- log.Fatal(err)
+ return "", err
  }
  version := argo.GetVersion()
  restConfig = restclient.AddUserAgent(restConfig, fmt.Sprintf("argo-workflows/%s argo-cli", version.Version))
  authString, err := kubeconfig.GetAuthString(restConfig, explicitPath)
  if err != nil {
- log.Fatal(err)
+ return "", err
  }
- return authString
+ return authString, nil
 }