CLI cleanup: exit-1 on error, and use consistent error printing every…

…where

Contains three major changes:
1. creates some standard "literally all CLIs" tooling in common/commoncli (to avoid conflicts with "common" and "cli")
2. moves `cli.NewApp()` and `PrintableError` into commoncli (as `New` and `Problem`)
3. fancy shmancy wrapped error printing with indentation, nested-error cleanup, and colors (we had colors before)

1 and 2 are pretty straightforward, and I've added a simple lint to block using urfave directly to make CLIs (to prevent future accidents).

3 is an attempt at "what if our error messages were similar but less bad".
And it also serves as an example of what you can do with enough force with wrapped errors.

Makefile

@@ -381,6 +381,8 @@ $(BUILD)/gomod-lint: go.mod internal/tools/go.mod common/archiver/gcloud/go.mod
 # it's a coarse "you probably don't need to re-lint" filter, nothing more.
 $(BUILD)/code-lint: $(LINT_SRC) $(BIN)/revive | $(BUILD)
 	$Q echo "lint..."
+	$Q # should probably be in a file but this is easier for now
+	$Q if grep -F 'cli.NewApp()' $(filter-out ./tools/common/commoncli/cli.go,$(LINT_SRC)); then echo -e "\nuse tools/common/commoncli instead\n" >&2 ; exit 1; fi
 	$Q # non-optional vet checks.  unfortunately these are not currently included in `go test`'s default behavior.
 	$Q go vet -copylocks ./... ./common/archiver/gcloud/...
 	$Q $(BIN)/revive -config revive.toml -exclude './vendor/...' -exclude './.gen/...' -formatter stylish ./...

cmd/bench/main.go

@@ -32,6 +32,7 @@ import (
 	"github.com/uber/cadence/bench"
 	"github.com/uber/cadence/bench/lib"
 	"github.com/uber/cadence/common/config"
+	"github.com/uber/cadence/tools/common/commoncli"
 )
 
 const (
@@ -101,10 +102,7 @@ func getZone(c *cli.Context) string {
 }
 
 func buildCLI() *cli.App {
-	app := cli.NewApp()
-	app.Name = "cadence-bench"
-	app.Usage = "Cadence bench"
-	app.Version = "0.0.1"
+	app := commoncli.New("cadence-bench", "Cadence bench", "")
 
 	app.Flags = []cli.Flag{
 		&cli.StringFlag{
@@ -152,8 +150,5 @@ func buildCLI() *cli.App {
 
 func main() {
 	app := buildCLI()
-	if err := app.Run(os.Args); err != nil {
-		_, _ = fmt.Fprintln(app.ErrWriter, err)
-		os.Exit(1)
-	}
+	_ = app.Run(os.Args) // exits on error
 }

cmd/canary/main.go

@@ -31,6 +31,7 @@ import (
 
 	"github.com/uber/cadence/canary"
 	"github.com/uber/cadence/common/config"
+	"github.com/uber/cadence/tools/common/commoncli"
 )
 
 func startHandler(c *cli.Context) error {
@@ -87,10 +88,7 @@ func getZone(c *cli.Context) string {
 }
 
 func buildCLI() *cli.App {
-	app := cli.NewApp()
-	app.Name = "cadence-canary"
-	app.Usage = "Cadence canary"
-	app.Version = "0.0.1"
+	app := commoncli.New("cadence-canary", "Cadence canary", "")
 
 	app.Flags = []cli.Flag{
 		&cli.StringFlag{
@@ -147,8 +145,5 @@ func buildCLI() *cli.App {
 
 func main() {
 	app := buildCLI()
-	if err := app.Run(os.Args); err != nil {
-		_, _ = fmt.Fprintln(app.ErrWriter, err)
-		os.Exit(1)
-	}
+	_ = app.Run(os.Args) // exits on error
 }

cmd/server/cadence/cadence.go

@@ -37,6 +37,7 @@ import (
 	"github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql"
 	"github.com/uber/cadence/common/service"
 	"github.com/uber/cadence/tools/cassandra"
+	"github.com/uber/cadence/tools/common/commoncli"
 	"github.com/uber/cadence/tools/sql"
 )
 
@@ -168,10 +169,7 @@ func BuildCLI(releaseVersion string, gitRevision string) *cli.App {
 		"   Note:  Feature version is for compatibility checking between server and clients if enabled feature checking. Server is always backward compatible to older CLI versions, but not accepting newer than it can support.",
 		releaseVersion, gitRevision, client.SupportedCLIVersion, client.SupportedGoSDKVersion, client.SupportedJavaSDKVersion)
 
-	app := cli.NewApp()
-	app.Name = "cadence"
-	app.Usage = "Cadence server"
-	app.Version = version
+	app := commoncli.New("cadence", "Cadence server", version)
 
 	app.Flags = []cli.Flag{
 		&cli.StringFlag{

cmd/server/main.go

@@ -21,7 +21,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 
 	"github.com/uber/cadence/cmd/server/cadence"
@@ -38,8 +37,5 @@ import (
 // main entry point for the cadence server
 func main() {
 	app := cadence.BuildCLI(metrics.ReleaseVersion, metrics.Revision)
-	if err := app.Run(os.Args); err != nil {
-		_, _ = fmt.Fprintln(app.ErrWriter, err)
-		os.Exit(1)
-	}
+	_ = app.Run(os.Args) // exits on error
 }

cmd/tools/cassandra/main.go

@@ -21,7 +21,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 
 	"github.com/uber/cadence/tools/cassandra"
@@ -30,9 +29,5 @@ import (
 )
 
 func main() {
-	err := cassandra.RunTool(os.Args)
-	if err != nil {
-		_, _ = fmt.Fprintln(os.Stderr, err)
-		os.Exit(1)
-	}
+	_ = cassandra.BuildCLIOptions().Run(os.Args) // exits on error
 }

cmd/tools/cli/main.go

@@ -21,7 +21,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 
 	"github.com/uber/cadence/tools/cli"
@@ -36,8 +35,5 @@ import (
 // See cadence/tools/cli/README.md for usage
 func main() {
 	app := cli.NewCliApp()
-	if err := app.Run(os.Args); err != nil {
-		_, _ = fmt.Fprintln(app.ErrWriter, err)
-		os.Exit(1)
-	}
+	_ = app.Run(os.Args) // exits on error
 }

cmd/tools/sql/main.go

@@ -21,7 +21,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 
 	"github.com/uber/cadence/tools/sql"
@@ -31,9 +30,5 @@ import (
 )
 
 func main() {
-	err := sql.RunTool(os.Args)
-	if err != nil {
-		_, _ = fmt.Fprintln(os.Stderr, err)
-		os.Exit(1)
-	}
+	_ = sql.RunTool(os.Args) // exits on error
 }

tools/cassandra/main.go

@@ -26,13 +26,14 @@ import (
 	"github.com/urfave/cli/v2"
 
 	"github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql"
+	"github.com/uber/cadence/tools/common/commoncli"
 	"github.com/uber/cadence/tools/common/schema"
 )
 
 // RunTool runs the cadence-cassandra-tool command line tool
 func RunTool(args []string) error {
 	app := BuildCLIOptions()
-	return app.Run(args)
+	return app.Run(args) // exits on error
 }
 
 // SetupSchema setups the cassandra schema
@@ -62,11 +63,7 @@ func cliHandler(c *cli.Context, handler func(c *cli.Context) error) error {
 }
 
 func BuildCLIOptions() *cli.App {
-
-	app := cli.NewApp()
-	app.Name = "cadence-cassandra-tool"
-	app.Usage = "Command line tool for cadence cassandra operations"
-	app.Version = "0.0.1"
+	app := commoncli.New("cadence-cassandra-tool", "Command line tool for cadence cassandra operations", "")
 
 	app.Flags = []cli.Flag{
 		&cli.StringFlag{

tools/cli/admin_async_queue_commands.go

@@ -29,6 +29,7 @@ import (
 	"github.com/urfave/cli/v2"
 
 	"github.com/uber/cadence/common/types"
+	"github.com/uber/cadence/tools/common/commoncli"
 )
 
 func AdminGetAsyncWFConfig(c *cli.Context) error {
@@ -45,7 +46,7 @@ func AdminGetAsyncWFConfig(c *cli.Context) error {
 
 	resp, err := adminClient.GetDomainAsyncWorkflowConfiguraton(ctx, req)
 	if err != nil {
-		return PrintableError("Failed to get async wf queue config", err)
+		return commoncli.Problem("Failed to get async wf queue config", err)
 	}
 
 	if resp == nil || resp.Configuration == nil {
@@ -67,7 +68,7 @@ func AdminUpdateAsyncWFConfig(c *cli.Context) error {
 	var cfg types.AsyncWorkflowConfiguration
 	err := json.Unmarshal([]byte(asyncWFCfgJSON), &cfg)
 	if err != nil {
-		return PrintableError("Failed to parse async workflow config", err)
+		return commoncli.Problem("Failed to parse async workflow config", err)
 	}
 
 	ctx, cancel := newContext(c)
@@ -80,7 +81,7 @@ func AdminUpdateAsyncWFConfig(c *cli.Context) error {
 
 	_, err = adminClient.UpdateDomainAsyncWorkflowConfiguraton(ctx, req)
 	if err != nil {
-		return PrintableError("Failed to update async workflow queue config", err)
+		return commoncli.Problem("Failed to update async workflow queue config", err)
 	}
 
 	fmt.Printf("Successfully updated async workflow queue config for domain %s\n", domainName)

tools/cli/admin_cluster_commands.go

@@ -32,6 +32,7 @@ import (
 	"github.com/uber/cadence/common/types"
 	"github.com/uber/cadence/common/visibility"
 	"github.com/uber/cadence/service/worker/failovermanager"
+	"github.com/uber/cadence/tools/common/commoncli"
 )
 
 // An indirection for the prompt function so that it can be mocked in the unit tests
@@ -41,12 +42,12 @@ var promptFn = prompt
 func AdminAddSearchAttribute(c *cli.Context) error {
 	key := getRequiredOption(c, FlagSearchAttributesKey)
 	if err := visibility.ValidateSearchAttributeKey(key); err != nil {
-		return PrintableError("Invalid search-attribute key.", err)
+		return commoncli.Problem("Invalid search-attribute key.", err)
 	}
 
 	valType := getRequiredIntOption(c, FlagSearchAttributesType)
 	if !isValueTypeValid(valType) {
-		return PrintableError("Unknown Search Attributes value type.", nil)
+		return commoncli.Problem("Unknown Search Attributes value type.", nil)
 	}
 
 	// ask user for confirmation
@@ -66,7 +67,7 @@ func AdminAddSearchAttribute(c *cli.Context) error {
 
 	err := adminClient.AddSearchAttribute(ctx, request)
 	if err != nil {
-		return PrintableError("Add search attribute failed.", err)
+		return commoncli.Problem("Add search attribute failed.", err)
 	}
 	fmt.Println("Success. Note that for a multil-node Cadence cluster, DynamicConfig MUST be updated separately to whitelist the new attributes.")
 	return nil
@@ -80,7 +81,7 @@ func AdminDescribeCluster(c *cli.Context) error {
 	defer cancel()
 	response, err := adminClient.DescribeCluster(ctx)
 	if err != nil {
-		return PrintableError("Operation DescribeCluster failed.", err)
+		return commoncli.Problem("Operation DescribeCluster failed.", err)
 	}
 
 	prettyPrintJSONObject(response)
@@ -99,13 +100,13 @@ func AdminRebalanceStart(c *cli.Context) error {
 	}
 	input, err := json.Marshal(rbParams)
 	if err != nil {
-		return PrintableError("Failed to serialize params for failover workflow", err)
+		return commoncli.Problem("Failed to serialize params for failover workflow", err)
 	}
 	memo, err := getWorkflowMemo(map[string]interface{}{
 		common.MemoKeyForOperator: getOperator(),
 	})
 	if err != nil {
-		return PrintableError("Failed to serialize memo", err)
+		return commoncli.Problem("Failed to serialize memo", err)
 	}
 	request := &types.StartWorkflowExecutionRequest{
 		Domain:                              common.SystemLocalDomainName,
@@ -127,7 +128,7 @@ func AdminRebalanceStart(c *cli.Context) error {
 
 	resp, err := client.StartWorkflowExecution(tcCtx, request)
 	if err != nil {
-		return PrintableError("Failed to start failover workflow", err)
+		return commoncli.Problem("Failed to start failover workflow", err)
 	}
 	fmt.Println("Rebalance workflow started")
 	fmt.Println("wid: " + workflowID)