Switch to dependency injection for the main CLI (#6331)

Sets up some foundations for the coming coverage sprint:
- a standard way to inject dependencies into this CLI app (via `cli.App.Metadata`)
- a standard way to read and write CLI text (not yet used, but hopefully the intent is clear enough)
- many more error returns to cover the new branches

Next steps are roughly:
- write tests!
- remove all `fmt.*Print*` calls and use the appropriate output writer instead
  - this will expose more error branches: we should be using them.  standard outputs can fail if a pipe fails, and we should stop running when that occurs.
- remove all stdin-reading calls, and use the input reader instead
  - this will allow us to answer prompts / continue paging in tests
- move `ErrorAndExit`s over to error returns at some point
  - a lot of these really are "impossible" or are validated extremely early (CLI flags), so a panic equivalent is kinda reasonable for ergonomics
  - `os.Exit(1)` is much more restricting than panics, as it prevents running any defers (for printing or flushing to files) and cannot be checked in tests or suppressed for "attempt or use fallback"

---

I did try to use the `cli.Context.Context` field for more "normal" dependency injection... but it ended up more annoying.

The main issue it that requires using `app.RunContext(populatedContext)`, but direct access to the `*cli.App` exists _all over_ and urfave's API largely requires this.  That leaves a rather large hard-to-protect error path, and it takes a few additional awkward lines of code to construct and/or pass it around.

With `Metadata`, that construction can be done where all the other construction is done: inside `cli.App`-constructors.  And since the `Metadata` field is _completely_ ignored by urfave's internals currently, it's basically equivalent but easier.

And last but not least: if they *do* start printing Metadata some day or something, we can switch to context key(s) with relatively minimal fuss since they're both available in the same way in the same locations.

cmd/tools/cli/main.go

@@ -23,6 +23,8 @@ package main
 import (
 	"os"
 
+	"go.uber.org/zap"
+
 	"github.com/uber/cadence/tools/cli"
 	"github.com/uber/cadence/tools/common/commoncli"
 
@@ -35,6 +37,13 @@ import (
 // Start using this CLI tool with command
 // See cadence/tools/cli/README.md for usage
 func main() {
-	app := cli.NewCliApp()
+	app := cli.NewCliApp(cli.NewClientFactory(must(zap.NewDevelopment())))
 	commoncli.ExitHandler(app.Run(os.Args))
 }
+
+func must[T any](v T, err error) T {
+	if err != nil {
+		panic(err)
+	}
+	return v
+}

tools/cli/admin.go

@@ -368,15 +368,28 @@ func newAdminHistoryHostCommands() []*cli.Command {
 	}
 }
 
+// may be better to do this inside an "ensure setup" in each method,
+// but this reduces branches for testing.
+func withDomainClient(c *cli.Context, admin bool, cb func(dc *domainCLIImpl) error) error {
+	dc, err := newDomainCLI(c, admin)
+	if err != nil {
+		return err
+	}
+	return cb(dc)
+}
+
 func newAdminDomainCommands() []*cli.Command {
+
 	return []*cli.Command{
 		{
 			Name:    "register",
 			Aliases: []string{"re"},
 			Usage:   "Register workflow domain",
 			Flags:   adminRegisterDomainFlags,
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, true).RegisterDomain(c)
+				return withDomainClient(c, true, func(dc *domainCLIImpl) error {
+					return dc.RegisterDomain(c)
+				})
 			},
 		},
 		{
@@ -385,7 +398,9 @@ func newAdminDomainCommands() []*cli.Command {
 			Usage:   "Update existing workflow domain",
 			Flags:   adminUpdateDomainFlags,
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, true).UpdateDomain(c)
+				return withDomainClient(c, true, func(dc *domainCLIImpl) error {
+					return dc.UpdateDomain(c)
+				})
 			},
 		},
 		{
@@ -394,7 +409,9 @@ func newAdminDomainCommands() []*cli.Command {
 			Usage:   "Deprecate existing workflow domain",
 			Flags:   adminDeprecateDomainFlags,
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, true).DeprecateDomain(c)
+				return withDomainClient(c, true, func(dc *domainCLIImpl) error {
+					return dc.DeprecateDomain(c)
+				})
 			},
 		},
 		{
@@ -403,7 +420,9 @@ func newAdminDomainCommands() []*cli.Command {
 			Usage:   "Describe existing workflow domain",
 			Flags:   adminDescribeDomainFlags,
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, true).DescribeDomain(c)
+				return withDomainClient(c, true, func(dc *domainCLIImpl) error {
+					return dc.DescribeDomain(c)
+				})
 			},
 		},
 		{
@@ -460,7 +479,9 @@ func newAdminDomainCommands() []*cli.Command {
 				getFormatFlag(),
 			},
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, false).ListDomains(c)
+				return withDomainClient(c, false, func(dc *domainCLIImpl) error {
+					return dc.ListDomains(c)
+				})
 			},
 		},
 	}
@@ -741,7 +762,9 @@ func newAdminClusterCommands() []*cli.Command {
 				},
 			},
 			Action: func(c *cli.Context) error {
-				return newDomainCLI(c, false).FailoverDomains(c)
+				return withDomainClient(c, false, func(dc *domainCLIImpl) error {
+					return dc.FailoverDomains(c)
+				})
 			},
 		},
 		{

tools/cli/admin_async_queue_commands.go

@@ -33,7 +33,10 @@ import (
 )
 
 func AdminGetAsyncWFConfig(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	domainName := getRequiredOption(c, FlagDomain)
 
@@ -60,13 +63,16 @@ func AdminGetAsyncWFConfig(c *cli.Context) error {
 }
 
 func AdminUpdateAsyncWFConfig(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	domainName := getRequiredOption(c, FlagDomain)
 	asyncWFCfgJSON := getRequiredOption(c, FlagJSON)
 
 	var cfg types.AsyncWorkflowConfiguration
-	err := json.Unmarshal([]byte(asyncWFCfgJSON), &cfg)
+	err = json.Unmarshal([]byte(asyncWFCfgJSON), &cfg)
 	if err != nil {
 		return commoncli.Problem("Failed to parse async workflow config", err)
 	}

tools/cli/admin_cluster_commands.go

@@ -55,7 +55,10 @@ func AdminAddSearchAttribute(c *cli.Context) error {
 		color.YellowString(key), color.YellowString(intValTypeToString(valType)))
 	promptFn(promptMsg)
 
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 	ctx, cancel := newContext(c)
 	defer cancel()
 	request := &types.AddSearchAttributeRequest{
@@ -65,7 +68,7 @@ func AdminAddSearchAttribute(c *cli.Context) error {
 		SecurityToken: c.String(FlagSecurityToken),
 	}
 
-	err := adminClient.AddSearchAttribute(ctx, request)
+	err = adminClient.AddSearchAttribute(ctx, request)
 	if err != nil {
 		return commoncli.Problem("Add search attribute failed.", err)
 	}
@@ -75,7 +78,10 @@ func AdminAddSearchAttribute(c *cli.Context) error {
 
 // AdminDescribeCluster is used to dump information about the cluster
 func AdminDescribeCluster(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	ctx, cancel := newContext(c)
 	defer cancel()
@@ -89,7 +95,10 @@ func AdminDescribeCluster(c *cli.Context) error {
 }
 
 func AdminRebalanceStart(c *cli.Context) error {
-	client := getCadenceClient(c)
+	client, err := getCadenceClient(c)
+	if err != nil {
+		return err
+	}
 	tcCtx, cancel := newContext(c)
 	defer cancel()
 

tools/cli/admin_commands.go

@@ -165,7 +165,10 @@ func AdminDescribeWorkflow(c *cli.Context) error {
 }
 
 func describeMutableState(c *cli.Context) (*types.AdminDescribeWorkflowExecutionResponse, error) {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return nil, err
+	}
 
 	domain := getRequiredOption(c, FlagDomain)
 	wid := getRequiredOption(c, FlagWorkflowID)
@@ -196,7 +199,10 @@ func AdminMaintainCorruptWorkflow(c *cli.Context) error {
 	workflowID := c.String(FlagWorkflowID)
 	runID := c.String(FlagRunID)
 	skipErrors := c.Bool(FlagSkipErrorMode)
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	request := &types.AdminMaintainWorkflowRequest{
 		Domain: domainName,
@@ -209,7 +215,7 @@ func AdminMaintainCorruptWorkflow(c *cli.Context) error {
 
 	ctx, cancel := newContext(c)
 	defer cancel()
-	_, err := adminClient.MaintainCorruptWorkflow(ctx, request)
+	_, err = adminClient.MaintainCorruptWorkflow(ctx, request)
 	if err != nil {
 		return commoncli.Problem("Operation AdminMaintainCorruptWorkflow failed.", err)
 	}
@@ -233,7 +239,10 @@ func AdminDeleteWorkflow(c *cli.Context) error {
 	// useful if server is down somehow. However, we only support couple DB clients
 	// currently. If the server side hosts working, remote is a cleaner approach
 	if remote {
-		adminClient := cFactory.ServerAdminClient(c)
+		adminClient, err := getDeps(c).ServerAdminClient(c)
+		if err != nil {
+			return err
+		}
 		request := &types.AdminDeleteWorkflowRequest{
 			Domain: domain,
 			Execution: &types.WorkflowExecution{
@@ -242,7 +251,7 @@ func AdminDeleteWorkflow(c *cli.Context) error {
 			},
 			SkipErrors: skipError,
 		}
-		_, err := adminClient.DeleteWorkflow(ctx, request)
+		_, err = adminClient.DeleteWorkflow(ctx, request)
 		if err != nil {
 			return commoncli.Problem("Operation AdminMaintainCorruptWorkflow failed.", err)
 		}
@@ -380,7 +389,10 @@ func AdminGetShardID(c *cli.Context) error {
 
 // AdminRemoveTask describes history host
 func AdminRemoveTask(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	shardID := getRequiredIntOption(c, FlagShardID)
 	taskID := getRequiredInt64Option(c, FlagTaskID)
@@ -402,7 +414,7 @@ func AdminRemoveTask(c *cli.Context) error {
 		ClusterName:         clusterName,
 	}
 
-	err := adminClient.RemoveTask(ctx, req)
+	err = adminClient.RemoveTask(ctx, req)
 	if err != nil {
 		return commoncli.Problem("Remove task has failed", err)
 	}
@@ -461,7 +473,10 @@ func AdminSetShardRangeID(c *cli.Context) error {
 
 // AdminCloseShard closes shard by shard id
 func AdminCloseShard(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 	sid := getRequiredIntOption(c, FlagShardID)
 
 	ctx, cancel := newContext(c)
@@ -470,7 +485,7 @@ func AdminCloseShard(c *cli.Context) error {
 	req := &types.CloseShardRequest{}
 	req.ShardID = int32(sid)
 
-	err := adminClient.CloseShard(ctx, req)
+	err = adminClient.CloseShard(ctx, req)
 	if err != nil {
 		return commoncli.Problem("Close shard task has failed", err)
 	}
@@ -484,7 +499,10 @@ type ShardRow struct {
 
 // AdminDescribeShardDistribution describes shard distribution
 func AdminDescribeShardDistribution(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	ctx, cancel := newContext(c)
 	defer cancel()
@@ -529,7 +547,10 @@ func AdminDescribeShardDistribution(c *cli.Context) error {
 
 // AdminDescribeHistoryHost describes history host
 func AdminDescribeHistoryHost(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	wid := c.String(FlagWorkflowID)
 	sid := c.Int(FlagShardID)
@@ -568,7 +589,10 @@ func AdminDescribeHistoryHost(c *cli.Context) error {
 
 // AdminRefreshWorkflowTasks refreshes all the tasks of a workflow
 func AdminRefreshWorkflowTasks(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	domain := getRequiredOption(c, FlagDomain)
 	wid := getRequiredOption(c, FlagWorkflowID)
@@ -577,7 +601,7 @@ func AdminRefreshWorkflowTasks(c *cli.Context) error {
 	ctx, cancel := newContext(c)
 	defer cancel()
 
-	err := adminClient.RefreshWorkflowTasks(ctx, &types.RefreshWorkflowTasksRequest{
+	err = adminClient.RefreshWorkflowTasks(ctx, &types.RefreshWorkflowTasksRequest{
 		Domain: domain,
 		Execution: &types.WorkflowExecution{
 			WorkflowID: wid,
@@ -593,7 +617,10 @@ func AdminRefreshWorkflowTasks(c *cli.Context) error {
 
 // AdminResetQueue resets task processing queue states
 func AdminResetQueue(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	shardID := getRequiredIntOption(c, FlagShardID)
 	clusterName := getRequiredOption(c, FlagCluster)
@@ -608,7 +635,7 @@ func AdminResetQueue(c *cli.Context) error {
 		Type:        common.Int32Ptr(int32(typeID)),
 	}
 
-	err := adminClient.ResetQueue(ctx, req)
+	err = adminClient.ResetQueue(ctx, req)
 	if err != nil {
 		return commoncli.Problem("Failed to reset queue", err)
 	}
@@ -618,7 +645,10 @@ func AdminResetQueue(c *cli.Context) error {
 
 // AdminDescribeQueue describes task processing queue states
 func AdminDescribeQueue(c *cli.Context) error {
-	adminClient := cFactory.ServerAdminClient(c)
+	adminClient, err := getDeps(c).ServerAdminClient(c)
+	if err != nil {
+		return err
+	}
 
 	shardID := getRequiredIntOption(c, FlagShardID)
 	clusterName := getRequiredOption(c, FlagCluster)