From 25c21ab65e89d61c4958e00a9d02f1fcb87843ab Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 24 Sep 2021 10:54:18 +0200 Subject: [PATCH] cli: fix the process exit status codes Release note (bug fix): The exit status of the `cockroach` command did not follow the previously-documented table of exit status codes when an error occurred during the command start-up. (Only errors occuring after startup were reported using the correct code.) This defect has been fixed. This bug had existed ever since reference exit status codes had been introduced. --- pkg/cli/BUILD.bazel | 3 +++ pkg/cli/cli.go | 15 +++++++---- pkg/cli/cli_test.go | 27 +++++++++++++++++++ pkg/cli/clierror/formatted_error.go | 4 +++ pkg/cli/interactive_tests/test_server_sig.tcl | 4 +-- pkg/cli/testutils.go | 26 ++++++++++++++---- 6 files changed, 67 insertions(+), 12 deletions(-) diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel index 21ca6842c36e..f276769808e8 100644 --- a/pkg/cli/BUILD.bazel +++ b/pkg/cli/BUILD.bazel @@ -311,11 +311,14 @@ go_test( "//pkg/base", "//pkg/build", "//pkg/cli/clicfg", + "//pkg/cli/clierror", + "//pkg/cli/clierrorplus", "//pkg/cli/cliflags", "//pkg/cli/clisqlcfg", "//pkg/cli/clisqlclient", "//pkg/cli/clisqlexec", "//pkg/cli/democluster", + "//pkg/cli/exit", "//pkg/gossip", "//pkg/gossip/resolver", "//pkg/jobs", diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 083ce2824d03..86f266096c2d 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -67,16 +67,21 @@ func Main() { // Finally, extract the error code, as optionally specified // by the sub-command. - errCode = exit.UnspecifiedError() - var cliErr *clierror.Error - if errors.As(err, &cliErr) { - errCode = cliErr.GetExitCode() - } + errCode = getExitCode(err) } exit.WithCode(errCode) } +func getExitCode(err error) (errCode exit.Code) { + errCode = exit.UnspecifiedError() + var cliErr *clierror.Error + if errors.As(err, &cliErr) { + errCode = cliErr.GetExitCode() + } + return errCode +} + func doMain(cmd *cobra.Command, cmdName string) error { if cmd != nil { // Apply the configuration defaults from environment variables. diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index 45500e8873b7..84ff70ad23fd 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -14,9 +14,13 @@ import ( "fmt" "testing" + "github.com/cockroachdb/cockroach/pkg/cli/clierror" + "github.com/cockroachdb/cockroach/pkg/cli/clierrorplus" + "github.com/cockroachdb/cockroach/pkg/cli/exit" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/errors" + "github.com/spf13/cobra" ) func TestCLITimeout(t *testing.T) { @@ -72,3 +76,26 @@ func TestJunkPositionalArguments(t *testing.T) { } } } + +func Example_exitcode() { + c := NewCLITest(TestCLIParams{NoServer: true}) + defer c.Cleanup() + + testCmd := &cobra.Command{ + Use: "test-exit-code", + RunE: clierrorplus.MaybeDecorateError( + func(_ *cobra.Command, _ []string) error { + return clierror.NewError(errors.New("err"), exit.Interrupted()) + }), + } + cockroachCmd.AddCommand(testCmd) + defer cockroachCmd.RemoveCommand(testCmd) + + c.reportExitCode = true + c.Run("test-exit-code") + + // Output: + // test-exit-code + // ERROR: err + // exit code: 3 +} diff --git a/pkg/cli/clierror/formatted_error.go b/pkg/cli/clierror/formatted_error.go index ef4eaa4424f7..35c285e16083 100644 --- a/pkg/cli/clierror/formatted_error.go +++ b/pkg/cli/clierror/formatted_error.go @@ -57,6 +57,10 @@ type formattedError struct { showSeverity, verbose bool } +func (f *formattedError) Unwrap() error { + return f.err +} + // Error implements the error interface. func (f *formattedError) Error() string { // If we're applying recursively, ignore what's there and display the original error. diff --git a/pkg/cli/interactive_tests/test_server_sig.tcl b/pkg/cli/interactive_tests/test_server_sig.tcl index ca1247e716c9..4dee44b3f3a6 100644 --- a/pkg/cli/interactive_tests/test_server_sig.tcl +++ b/pkg/cli/interactive_tests/test_server_sig.tcl @@ -34,9 +34,9 @@ eexpect "shutdown completed" eexpect ":/# " end_test -start_test "Check that Ctrl+C finishes with exit code 1. (#9051)" +start_test "Check that Ctrl+C finishes with exit code 3. (#9051+#70673)" send "echo \$?\r" -eexpect "1\r\n" +eexpect "3\r\n" eexpect ":/# " end_test diff --git a/pkg/cli/testutils.go b/pkg/cli/testutils.go index bc6db3997098..1183452ed7e2 100644 --- a/pkg/cli/testutils.go +++ b/pkg/cli/testutils.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/cli/clierror" "github.com/cockroachdb/cockroach/pkg/cli/cliflags" "github.com/cockroachdb/cockroach/pkg/cli/clisqlexec" + "github.com/cockroachdb/cockroach/pkg/cli/exit" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/security/securitytest" @@ -61,15 +62,23 @@ type TestCLI struct { logScope *log.TestLogScope // if true, doesn't print args during RunWithArgs. omitArgs bool + // if true, prints the requested exit code during RunWithArgs. + reportExitCode bool } // TestCLIParams contains parameters used by TestCLI. type TestCLIParams struct { - T *testing.T - Insecure bool - NoServer bool - StoreSpecs []base.StoreSpec - Locality roachpb.Locality + T *testing.T + Insecure bool + // NoServer, if true, starts the test without a DB server. + NoServer bool + + // The store specifications for the in-memory server. + StoreSpecs []base.StoreSpec + // The locality tiers for the in-memory server. + Locality roachpb.Locality + + // NoNodelocal, if true, disables node-local external I/O storage. NoNodelocal bool } @@ -350,6 +359,13 @@ func (c TestCLI) RunWithArgs(origArgs []string) { return Run(args) }(); err != nil { clierror.OutputError(os.Stdout, err, true /*showSeverity*/, false /*verbose*/) + if c.reportExitCode { + fmt.Fprintln(os.Stdout, "exit code:", getExitCode(err)) + } + } else { + if c.reportExitCode { + fmt.Fprintln(os.Stdout, "exit code:", exit.Success()) + } } }