Skip to content

Commit

Permalink
CLI cleanup: exit-1 on error, and use consistent error printing every…
Browse files Browse the repository at this point in the history
…where (#6322)

Clean-up and follow-up from #6285 and #6318, but only part 1 of [probably 2].
The next part will change our ErrorAndExit calls into error returns, so we get chains of causes rather than just one message when a fatal error is hit.

If there's a part 3, maybe it'll use https://github.com/bracesdev/errtrace or a https://github.com/pkg/errors-like wrapper to collect useful stack traces along with the error returns, to bring back `CADENCE_CLI_SHOW_STACKS` for all errors.

---

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 accidentally using it in the future).

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.

---

Ultimately, this gives us (the ability to) have errors print like this:
```
Error: the most-recent cause of the problem
Error details:
  wrapped error
  details if they exist
  Error: another commoncli.Problem from further down the stack
  Error details:
    and other wrapped errors
    that it may contain
```
Rather than like this:
```
Error: the most-recent cause of the problem
Error details: wrapped error: details if they exist: another commoncli.Problem from further down the stack: and other wrapped errors: that it may contain
```
Because it looks "in the same spirit" as the printing before, but is hopefully more readable.
And hopefully it also helps show enough error-spelunking details that someone can figure out much fancier and more structured errors later, if desired.

This also ends up changing the main server binary to use this same fancy error printing, where before it did not:
```
❯ ./cadence-server start
2024/10/02 22:26:06 Loading config; env=development,zone=,configDir=./config
2024/10/02 22:26:06 Loading configFiles=[./config/base.yaml ./config/development.yaml]
2024/10/02 22:26:06 [WARN] dcRedirectionPolicy config is deprecated. Please replace it with clusterRedirectionPolicy.
2024/10/02 22:26:06 gocql: unable to dial control conn 127.0.0.1:9042: dial tcp 127.0.0.1:9042: connect: connection refused
Error: cassandra schema version compatibility check failed
Error details:
  creating CQL client
  gocql: unable to create session: control: unable to connect to initial hosts: dial tcp 127.0.0.1:9042: connect: connection refused
```
vs before:
```
❯ ./cadence-server start
2024/10/02 22:27:58 Loading config; env=development,zone=,configDir=./config
2024/10/02 22:27:58 Loading configFiles=[./config/base.yaml ./config/development.yaml]
2024/10/02 22:27:58 [WARN] dcRedirectionPolicy config is deprecated. Please replace it with clusterRedirectionPolicy.
2024/10/02 22:27:58 gocql: unable to dial control conn 127.0.0.1:9042: dial tcp 127.0.0.1:9042: connect: connection refused
cassandra schema version compatibility check failed: creating CQL client: gocql: unable to create session: control: unable to connect to initial hosts: dial tcp 127.0.0.1:9042: connect: connection refused
```
which seems like probably a positive change.  It is colorized, but fatih is smart enough to remove it when piped, and removing color from this binary all the time is fairly easy if we want.
  • Loading branch information
Groxx authored Oct 3, 2024
1 parent 80f4739 commit add84ef
Show file tree
Hide file tree
Showing 33 changed files with 607 additions and 267 deletions.
7 changes: 2 additions & 5 deletions cmd/bench/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -105,7 +106,6 @@ func buildCLI() *cli.App {
app.Name = "cadence-bench"
app.Usage = "Cadence bench"
app.Version = "0.0.1"

app.Flags = []cli.Flag{
&cli.StringFlag{
Name: flagRoot,
Expand Down Expand Up @@ -152,8 +152,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)
}
commoncli.ExitHandler(app.Run(os.Args))
}
7 changes: 2 additions & 5 deletions cmd/canary/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -91,7 +92,6 @@ func buildCLI() *cli.App {
app.Name = "cadence-canary"
app.Usage = "Cadence canary"
app.Version = "0.0.1"

app.Flags = []cli.Flag{
&cli.StringFlag{
Name: "root",
Expand Down Expand Up @@ -147,8 +147,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)
}
commoncli.ExitHandler(app.Run(os.Args))
}
1 change: 0 additions & 1 deletion cmd/server/cadence/cadence.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ func BuildCLI(releaseVersion string, gitRevision string) *cli.App {
app.Name = "cadence"
app.Usage = "Cadence server"
app.Version = version

app.Flags = []cli.Flag{
&cli.StringFlag{
Name: "root",
Expand Down
7 changes: 6 additions & 1 deletion cmd/server/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ require (
github.com/uber/cadence/common/archiver/gcloud v0.0.0-00010101000000-000000000000
)

require github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1 // indirect
require (
github.com/fatih/color v1.13.0 // indirect
github.com/mattn/go-colorable v0.1.9 // indirect
github.com/mattn/go-isatty v0.0.14 // indirect
github.com/xrash/smetrics v0.0.0-20240521201337-686a1a2994c1 // indirect
)

require (
cloud.google.com/go v0.110.8 // indirect
Expand Down
10 changes: 10 additions & 0 deletions cmd/server/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ github.com/envoyproxy/go-control-plane v0.9.10-0.20210907150352-cf90f659a021/go.
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a h1:yDWHCSQ40h88yih2JAcL6Ls/kVkSE8GFACTGVnMPruw=
github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a/go.mod h1:7Ga40egUymuWXxAe151lTNnCv97MddSOVsjpPPkityA=
github.com/fatih/color v1.13.0 h1:8LOYc1KYPPmyKMuN8QV2DNRWNbLo6LZ0iLs8+mlH53w=
github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk=
github.com/fatih/structtag v1.0.0/go.mod h1:IKitwq45uXL/yqi5mYghiD3w9H6eTOvI9vnk8tXMphA=
github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4=
github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94=
Expand Down Expand Up @@ -285,6 +287,11 @@ github.com/m3db/prometheus_procfs v0.8.1 h1:LsxWzVELhDU9sLsZTaFLCeAwCn7bC7qecZcK
github.com/m3db/prometheus_procfs v0.8.1/go.mod h1:N8lv8fLh3U3koZx1Bnisj60GYUMDpWb09x1R+dmMOJo=
github.com/mailru/easyjson v0.7.6 h1:8yTIVnZgCoiM1TgqoeTl+LfU5Jg6/xL3QhGQnimLYnA=
github.com/mailru/easyjson v0.7.6/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
github.com/mattn/go-colorable v0.1.9 h1:sqDoxXbdeALODt0DAeJCVp38ps9ZogZEAXjus69YV3U=
github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Kysco4FUpU=
github.com/mattn/go-isatty v0.0.14 h1:yVuAays6BHfxijgZPzw+3Zlu5yQgKGP2/hcQbHb7S9Y=
github.com/mattn/go-isatty v0.0.14/go.mod h1:7GGIvUiUoEMVVmxf/4nioHXj79iQHKdU27kJ6hsGG94=
github.com/mattn/go-shellwords v1.0.10/go.mod h1:EZzvwXDESEeg03EKmM+RmDnNOPKG4lLtQsUlTZDWQ8Y=
github.com/mattn/go-sqlite3 v1.9.0/go.mod h1:FPy6KqzDD04eiIsT53CuJW3U88zkxoIYsOqkbpncsNc=
github.com/mattn/go-sqlite3 v1.11.0 h1:LDdKkqtYlom37fkvqs8rMPFKAMe8+SgjbwZ6ex1/A/Q=
Expand Down Expand Up @@ -562,9 +569,11 @@ golang.org/x/sys v0.0.0-20190502145724-3ef323f4f1fd/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20191220142924-d4481acd189f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200106162015-b016eb3dc98e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200117145432-59e60aa80a0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200122134326-e047566fdf82/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200212091648-12a6c2dcc1e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200615200032-f1bc736245b1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Expand All @@ -576,6 +585,7 @@ golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20210603081109-ebe580a85c40/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Expand Down
7 changes: 2 additions & 5 deletions cmd/server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
package main

import (
"fmt"
"os"

"github.com/uber/cadence/cmd/server/cadence"
"github.com/uber/cadence/common/metrics"
"github.com/uber/cadence/tools/common/commoncli"

_ "github.com/uber/cadence/common/archiver/gcloud" // needed to load the optional gcloud archiver plugin
_ "github.com/uber/cadence/common/asyncworkflow/queue/kafka" // needed to load kafka asyncworkflow queue
Expand All @@ -38,8 +38,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)
}
commoncli.ExitHandler(app.Run(os.Args))
}
9 changes: 3 additions & 6 deletions cmd/tools/cassandra/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,15 @@
package main

import (
"fmt"
"os"

"github.com/uber/cadence/tools/cassandra"
"github.com/uber/cadence/tools/common/commoncli"

_ "github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql/public" // needed to load the default gocql client
)

func main() {
err := cassandra.RunTool(os.Args)
if err != nil {
_, _ = fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
app := cassandra.BuildCLIOptions()
commoncli.ExitHandler(app.Run(os.Args))
}
7 changes: 2 additions & 5 deletions cmd/tools/cli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
package main

import (
"fmt"
"os"

"github.com/uber/cadence/tools/cli"
"github.com/uber/cadence/tools/common/commoncli"

_ "github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra" // needed to load cassandra plugin
_ "github.com/uber/cadence/common/persistence/nosql/nosqlplugin/cassandra/gocql/public" // needed to load the default gocql client
Expand All @@ -36,8 +36,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)
}
commoncli.ExitHandler(app.Run(os.Args))
}
8 changes: 2 additions & 6 deletions cmd/tools/sql/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,15 @@
package main

import (
"fmt"
"os"

"github.com/uber/cadence/tools/common/commoncli"
"github.com/uber/cadence/tools/sql"

_ "github.com/uber/cadence/common/persistence/sql/sqlplugin/mysql" // needed to load mysql plugin
_ "github.com/uber/cadence/common/persistence/sql/sqlplugin/postgres" // needed to load postgres plugin
)

func main() {
err := sql.RunTool(os.Args)
if err != nil {
_, _ = fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
commoncli.ExitHandler(sql.RunTool(os.Args))
}
2 changes: 1 addition & 1 deletion tools/cassandra/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
// 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
Expand Down
7 changes: 4 additions & 3 deletions tools/cli/admin_async_queue_commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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)
Expand Down
15 changes: 8 additions & 7 deletions tools/cli/admin_cluster_commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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,
Expand All @@ -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)
Expand Down
Loading

0 comments on commit add84ef

Please sign in to comment.