From a7d43e3272a2114061f2eaaf1f756f7af68969ab Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 3 Dec 2024 14:00:29 +0100 Subject: [PATCH 1/2] cli: deprecate `cli.StatusError` direct field usage The exported `cli.StatusError` type may be used by some directly within their own projects, making it difficult to update the struct's fields. This patch converts the exported `cli.StatusError` to an interface instead, so that code wrapping the CLI would still be able to match the error and get the status code without exposing the fields. This is a breaking change for those relying on creating a `cli.StatusError{}` and accessing the error's fields. For those using `errors.As(err, &statusError)` and `err.(cli.StatusError)` will be able to continue using it without breakage. Users accessing the fields of `cli.StatusError{}.StatusCode` would need to use the new `GetStatusCode()` method. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli-plugins/plugin/plugin.go | 8 +++---- cli/cobra.go | 5 ++-- cli/command/config/inspect.go | 3 ++- cli/command/container/attach.go | 3 ++- cli/command/container/attach_test.go | 4 ++-- cli/command/container/create.go | 13 +++++----- cli/command/container/create_test.go | 4 ++-- cli/command/container/exec.go | 5 ++-- cli/command/container/exec_test.go | 4 ++-- cli/command/container/run.go | 31 ++++++++++++------------ cli/command/container/run_test.go | 18 +++++++------- cli/command/container/start.go | 3 ++- cli/command/image/build.go | 3 ++- cli/command/inspect/inspector.go | 8 +++---- cli/command/network/remove.go | 3 ++- cli/command/node/inspect.go | 3 ++- cli/command/secret/inspect.go | 3 ++- cli/command/service/inspect.go | 3 ++- cli/command/system/events.go | 5 ++-- cli/command/system/info.go | 5 ++-- cli/command/system/version.go | 3 ++- cli/error.go | 25 ++++++++++--------- cmd/docker/docker.go | 9 +++---- internal/status_error.go | 36 ++++++++++++++++++++++++++++ 24 files changed, 128 insertions(+), 79 deletions(-) create mode 100644 internal/status_error.go diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index cf57aad5e17c..a4c1d6363129 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -14,6 +14,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/connhelper" "github.com/docker/cli/cli/debug" + "github.com/docker/cli/internal" "github.com/docker/docker/client" "github.com/spf13/cobra" "go.opentelemetry.io/otel" @@ -93,14 +94,11 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { plugin := makeCmd(dockerCli) if err := RunPlugin(dockerCli, plugin, meta); err != nil { - var stErr cli.StatusError + var stErr internal.StatusError if errors.As(err, &stErr) { + _, _ = fmt.Fprintln(dockerCli.Err(), stErr) // StatusError should only be used for errors, and all errors should // have a non-zero exit status, so never exit with 0 - if stErr.StatusCode == 0 { // FIXME(thaJeztah): this should never be used with a zero status-code. Check if we do this anywhere. - stErr.StatusCode = 1 - } - _, _ = fmt.Fprintln(dockerCli.Err(), stErr) os.Exit(stErr.StatusCode) } _, _ = fmt.Fprintln(dockerCli.Err(), err) diff --git a/cli/cobra.go b/cli/cobra.go index feab2b4a91c8..6c6982a318eb 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -10,6 +10,7 @@ import ( pluginmanager "github.com/docker/cli/cli-plugins/manager" "github.com/docker/cli/cli/command" cliflags "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/docker/docker/pkg/homedir" "github.com/docker/docker/registry" "github.com/fvbommel/sortorder" @@ -92,8 +93,8 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error { return nil } - return StatusError{ - Status: fmt.Sprintf("%s\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), + return internal.StatusError{ + Cause: fmt.Errorf("%w\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()), StatusCode: 125, } } diff --git a/cli/command/config/inspect.go b/cli/command/config/inspect.go index 7ae4a4c40435..aa43298dab86 100644 --- a/cli/command/config/inspect.go +++ b/cli/command/config/inspect.go @@ -12,6 +12,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/formatter" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/spf13/cobra" ) @@ -67,7 +68,7 @@ func RunConfigInspect(ctx context.Context, dockerCli command.Cli, opts InspectOp } if err := InspectFormatWrite(configCtx, opts.Names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return internal.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index a4dbe0ec0d11..ddbaa084a9b6 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -7,6 +7,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/docker/cli/internal" "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/moby/sys/signal" @@ -161,7 +162,7 @@ func getExitStatus(errC <-chan error, resultC <-chan container.WaitResponse) err return errors.New(result.Error.Message) } if result.StatusCode != 0 { - return cli.StatusError{StatusCode: int(result.StatusCode)} + return internal.StatusError{StatusCode: int(result.StatusCode)} } case err := <-errC: return err diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go index 5b0276f38ccf..3212d48a0829 100644 --- a/cli/command/container/attach_test.go +++ b/cli/command/container/attach_test.go @@ -4,7 +4,7 @@ import ( "io" "testing" - "github.com/docker/cli/cli" + "github.com/docker/cli/internal" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types/container" "github.com/pkg/errors" @@ -110,7 +110,7 @@ func TestGetExitStatus(t *testing.T) { result: &container.WaitResponse{ StatusCode: 15, }, - expectedError: cli.StatusError{StatusCode: 15}, + expectedError: internal.StatusError{StatusCode: 15}, }, } diff --git a/cli/command/container/create.go b/cli/command/container/create.go index be81c2173311..4571fde65018 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -15,6 +15,7 @@ import ( "github.com/docker/cli/cli/command/image" "github.com/docker/cli/cli/internal/jsonstream" "github.com/docker/cli/cli/streams" + "github.com/docker/cli/internal" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/container" imagetypes "github.com/docker/docker/api/types/image" @@ -92,8 +93,8 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command { func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error { if err := validatePullOpt(options.pull); err != nil { - return cli.StatusError{ - Status: withHelp(err, "create").Error(), + return internal.StatusError{ + Cause: withHelp(err, "create"), StatusCode: 125, } } @@ -109,14 +110,14 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, copts.env = *opts.NewListOptsRef(&newEnv, nil) containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) if err != nil { - return cli.StatusError{ - Status: withHelp(err, "create").Error(), + return internal.StatusError{ + Cause: withHelp(err, "create"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil { - return cli.StatusError{ - Status: withHelp(err, "create").Error(), + return internal.StatusError{ + Cause: withHelp(err, "create"), StatusCode: 125, } } diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index c02ed14fb5b9..acbcb1ca4f7d 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -10,8 +10,8 @@ import ( "strings" "testing" - "github.com/docker/cli/cli" "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/internal" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/notary" "github.com/docker/docker/api/types/container" @@ -185,7 +185,7 @@ func TestCreateContainerImagePullPolicyInvalid(t *testing.T) { &containerOptions{}, ) - statusErr := cli.StatusError{} + var statusErr internal.StatusError assert.Check(t, errors.As(err, &statusErr)) assert.Check(t, is.Equal(statusErr.StatusCode, 125)) assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) diff --git a/cli/command/container/exec.go b/cli/command/container/exec.go index c2a1d447998b..759c039789fe 100644 --- a/cli/command/container/exec.go +++ b/cli/command/container/exec.go @@ -9,6 +9,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/internal" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" @@ -206,11 +207,11 @@ func getExecExitStatus(ctx context.Context, apiClient client.ContainerAPIClient, if !client.IsErrConnectionFailed(err) { return err } - return cli.StatusError{StatusCode: -1} + return internal.StatusError{StatusCode: -1} } status := resp.ExitCode if status != 0 { - return cli.StatusError{StatusCode: status} + return internal.StatusError{StatusCode: status} } return nil } diff --git a/cli/command/container/exec_test.go b/cli/command/container/exec_test.go index 89193b29f487..e9e7a8c695d3 100644 --- a/cli/command/container/exec_test.go +++ b/cli/command/container/exec_test.go @@ -6,8 +6,8 @@ import ( "os" "testing" - "github.com/docker/cli/cli" "github.com/docker/cli/cli/config/configfile" + "github.com/docker/cli/internal" "github.com/docker/cli/internal/test" "github.com/docker/cli/opts" "github.com/docker/docker/api/types" @@ -231,7 +231,7 @@ func TestGetExecExitStatus(t *testing.T) { }, { exitCode: 15, - expectedError: cli.StatusError{StatusCode: 15}, + expectedError: internal.StatusError{StatusCode: 15}, }, } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 5ef3e8b7bf7b..4429672db7a6 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -10,6 +10,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/docker/cli/internal" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/container" "github.com/moby/sys/signal" @@ -84,8 +85,8 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command { func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error { if err := validatePullOpt(ropts.pull); err != nil { - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 125, } } @@ -102,14 +103,14 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType) // just in case the parse does not exit if err != nil { - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 125, } } if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil { - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 125, } } @@ -241,7 +242,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } status := <-statusChan if status != 0 { - return cli.StatusError{StatusCode: status} + return internal.StatusError{StatusCode: status} } case status := <-statusChan: // notify hijackedIOStreamer that we're exiting and wait @@ -249,7 +250,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption cancelFun() <-errCh if status != 0 { - return cli.StatusError{StatusCode: status} + return internal.StatusError{StatusCode: status} } } @@ -311,7 +312,7 @@ func withHelp(err error, commandName string) error { // toStatusError attempts to detect specific error-conditions to assign // an appropriate exit-code for situations where the problem originates -// from the container. It returns [cli.StatusError] with the original +// from the container. It returns [internal.StatusError] with the original // error message and the Status field set as follows: // // - 125: for generic failures sent back from the daemon @@ -323,21 +324,21 @@ func toStatusError(err error) error { errMsg := err.Error() if strings.Contains(errMsg, "executable file not found") || strings.Contains(errMsg, "no such file or directory") || strings.Contains(errMsg, "system cannot find the file specified") { - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 127, } } if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) { - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 126, } } - return cli.StatusError{ - Status: withHelp(err, "run").Error(), + return internal.StatusError{ + Cause: withHelp(err, "run"), StatusCode: 125, } } diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 4c2ced4f564f..1b759616c08a 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -12,8 +12,8 @@ import ( "time" "github.com/creack/pty" - "github.com/docker/cli/cli" "github.com/docker/cli/cli/streams" + "github.com/docker/cli/internal" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/notary" "github.com/docker/docker/api/types" @@ -131,7 +131,7 @@ func TestRunAttach(t *testing.T) { select { case cmdErr := <-cmdErrC: - assert.Equal(t, cmdErr, cli.StatusError{ + assert.Equal(t, cmdErr, internal.StatusError{ StatusCode: 33, }) case <-time.After(2 * time.Second): @@ -213,7 +213,7 @@ func TestRunAttachTermination(t *testing.T) { select { case cmdErr := <-cmdErrC: - assert.Equal(t, cmdErr, cli.StatusError{ + assert.Equal(t, cmdErr, internal.StatusError{ StatusCode: 130, }) case <-time.After(2 * time.Second): @@ -289,10 +289,10 @@ func TestRunPullTermination(t *testing.T) { select { case cmdErr := <-cmdErrC: - assert.Equal(t, cmdErr, cli.StatusError{ - StatusCode: 125, - Status: "docker: context canceled\n\nRun 'docker run --help' for more information", - }) + assert.ErrorIs(t, cmdErr, context.Canceled) + v, ok := cmdErr.(internal.StatusError) + assert.Check(t, ok) + assert.Check(t, is.Equal(v.StatusCode, 125)) case <-time.After(10 * time.Second): t.Fatal("cmd did not return before the timeout") } @@ -342,7 +342,7 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) { cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() - statusErr := cli.StatusError{} + statusErr := internal.StatusError{} assert.Check(t, errors.As(err, &statusErr)) assert.Check(t, is.Equal(statusErr.StatusCode, 125)) assert.Check(t, is.ErrorContains(err, tc.expectedError)) @@ -375,7 +375,7 @@ func TestRunContainerImagePullPolicyInvalid(t *testing.T) { &containerOptions{}, ) - statusErr := cli.StatusError{} + statusErr := internal.StatusError{} assert.Check(t, errors.As(err, &statusErr)) assert.Check(t, is.Equal(statusErr.StatusCode, 125)) assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg)) diff --git a/cli/command/container/start.go b/cli/command/container/start.go index 29aaa988d6b8..d9a58503fbd8 100644 --- a/cli/command/container/start.go +++ b/cli/command/container/start.go @@ -9,6 +9,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/docker/cli/internal" "github.com/docker/docker/api/types/container" "github.com/moby/sys/signal" "github.com/moby/term" @@ -172,7 +173,7 @@ func RunStart(ctx context.Context, dockerCli command.Cli, opts *StartOptions) er } if status := <-statusChan; status != 0 { - return cli.StatusError{StatusCode: status} + return internal.StatusError{StatusCode: status} } return nil case opts.Checkpoint != "": diff --git a/cli/command/image/build.go b/cli/command/image/build.go index f8355fbe96af..bf7a0177a230 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -22,6 +22,7 @@ import ( "github.com/docker/cli/cli/command/image/build" "github.com/docker/cli/cli/internal/jsonstream" "github.com/docker/cli/cli/streams" + "github.com/docker/cli/internal" "github.com/docker/cli/opts" "github.com/docker/docker/api" "github.com/docker/docker/api/types" @@ -372,7 +373,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) if options.quiet { fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff) } - return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code} + return internal.StatusError{Cause: jerr, StatusCode: jerr.Code} } return err } diff --git a/cli/command/inspect/inspector.go b/cli/command/inspect/inspector.go index 46ba8750d6a4..7904abbcfe3c 100644 --- a/cli/command/inspect/inspector.go +++ b/cli/command/inspect/inspector.go @@ -10,7 +10,7 @@ import ( "strings" "text/template" - "github.com/docker/cli/cli" + "github.com/docker/cli/internal" "github.com/docker/cli/templates" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -67,7 +67,7 @@ type GetRefFunc func(ref string) (any, []byte, error) func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFunc) error { inspector, err := NewTemplateInspectorFromString(out, tmplStr) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return internal.StatusError{StatusCode: 64, Cause: err} } var inspectErrs []string @@ -88,9 +88,9 @@ func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFu } if len(inspectErrs) != 0 { - return cli.StatusError{ + return internal.StatusError{ StatusCode: 1, - Status: strings.Join(inspectErrs, "\n"), + Cause: errors.New(strings.Join(inspectErrs, "\n")), } } return nil diff --git a/cli/command/network/remove.go b/cli/command/network/remove.go index 105e629f614c..d6d2847f364e 100644 --- a/cli/command/network/remove.go +++ b/cli/command/network/remove.go @@ -7,6 +7,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/docker/cli/internal" "github.com/docker/docker/api/types/network" "github.com/docker/docker/errdefs" "github.com/spf13/cobra" @@ -68,7 +69,7 @@ func runRemove(ctx context.Context, dockerCli command.Cli, networks []string, op } if status != 0 { - return cli.StatusError{StatusCode: status} + return internal.StatusError{StatusCode: status} } return nil } diff --git a/cli/command/node/inspect.go b/cli/command/node/inspect.go index 270a14bd2bdd..823d78bb9403 100644 --- a/cli/command/node/inspect.go +++ b/cli/command/node/inspect.go @@ -12,6 +12,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/formatter" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/spf13/cobra" ) @@ -69,7 +70,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(nodeCtx, opts.nodeIds, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return internal.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/secret/inspect.go b/cli/command/secret/inspect.go index 5559d43d383e..3a0f38af547e 100644 --- a/cli/command/secret/inspect.go +++ b/cli/command/secret/inspect.go @@ -12,6 +12,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/formatter" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/spf13/cobra" ) @@ -65,7 +66,7 @@ func runSecretInspect(ctx context.Context, dockerCli command.Cli, opts inspectOp } if err := InspectFormatWrite(secretCtx, opts.names, getRef); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return internal.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/service/inspect.go b/cli/command/service/inspect.go index 28b957f77310..9b5f1bc0388d 100644 --- a/cli/command/service/inspect.go +++ b/cli/command/service/inspect.go @@ -11,6 +11,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/formatter" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/network" "github.com/docker/docker/errdefs" @@ -94,7 +95,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, opts inspectOptions) } if err := InspectFormatWrite(serviceCtx, opts.refs, getRef, getNetwork); err != nil { - return cli.StatusError{StatusCode: 1, Status: err.Error()} + return internal.StatusError{StatusCode: 1, Cause: err} } return nil } diff --git a/cli/command/system/events.go b/cli/command/system/events.go index 9b5f965e5b3c..426bb492cbca 100644 --- a/cli/command/system/events.go +++ b/cli/command/system/events.go @@ -14,6 +14,7 @@ import ( "github.com/docker/cli/cli/command/completion" "github.com/docker/cli/cli/command/formatter" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/docker/cli/opts" "github.com/docker/cli/templates" "github.com/docker/docker/api/types/events" @@ -58,9 +59,9 @@ func NewEventsCommand(dockerCli command.Cli) *cobra.Command { func runEvents(ctx context.Context, dockerCli command.Cli, options *eventsOptions) error { tmpl, err := makeTemplate(options.format) if err != nil { - return cli.StatusError{ + return internal.StatusError{ StatusCode: 64, - Status: "Error parsing format: " + err.Error(), + Cause: fmt.Errorf("error parsing format: %w", err), } } ctx, cancel := context.WithCancel(ctx) diff --git a/cli/command/system/info.go b/cli/command/system/info.go index 108a65a5f1f4..518f3c9d0478 100644 --- a/cli/command/system/info.go +++ b/cli/command/system/info.go @@ -19,6 +19,7 @@ import ( "github.com/docker/cli/cli/command/formatter" "github.com/docker/cli/cli/debug" flagsHelper "github.com/docker/cli/cli/flags" + "github.com/docker/cli/internal" "github.com/docker/cli/templates" "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/api/types/system" @@ -457,9 +458,9 @@ func formatInfo(output io.Writer, info dockerInfo, format string) error { tmpl, err := templates.Parse(format) if err != nil { - return cli.StatusError{ + return internal.StatusError{ StatusCode: 64, - Status: "template parsing error: " + err.Error(), + Cause: fmt.Errorf("template parsing error: %w", err), } } err = tmpl.Execute(output, info) diff --git a/cli/command/system/version.go b/cli/command/system/version.go index f25446b06dd8..eeb56e629697 100644 --- a/cli/command/system/version.go +++ b/cli/command/system/version.go @@ -15,6 +15,7 @@ import ( "github.com/docker/cli/cli/command/formatter/tabwriter" flagsHelper "github.com/docker/cli/cli/flags" "github.com/docker/cli/cli/version" + "github.com/docker/cli/internal" "github.com/docker/cli/templates" "github.com/docker/docker/api/types" "github.com/pkg/errors" @@ -148,7 +149,7 @@ func runVersion(ctx context.Context, dockerCli command.Cli, opts *versionOptions var err error tmpl, err := newVersionTemplate(opts.format) if err != nil { - return cli.StatusError{StatusCode: 64, Status: err.Error()} + return internal.StatusError{StatusCode: 64, Cause: err} } // TODO print error if kubernetes is used? diff --git a/cli/error.go b/cli/error.go index 8c4a5f952bce..73ed2edf08d4 100644 --- a/cli/error.go +++ b/cli/error.go @@ -1,21 +1,20 @@ package cli import ( - "strconv" + "github.com/docker/cli/internal" ) // StatusError reports an unsuccessful exit by a command. -type StatusError struct { - Status string - StatusCode int -} +type StatusError interface { + Error() string + Unwrap() error -// Error formats the error for printing. If a custom Status is provided, -// it is returned as-is, otherwise it generates a generic error-message -// based on the StatusCode. -func (e StatusError) Error() string { - if e.Status == "" { - return "exit status " + strconv.Itoa(e.StatusCode) - } - return e.Status + // GetStatusCode returns the status code of the error. + // The status code will never be 0. + GetStatusCode() int } + +// Pin the exported StatusError interface to the internal.StatusError type. +// This is necessary to ensure that the internal.StatusError type does +// not break the compatibility of the exported interface. +var _ StatusError = internal.StatusError{} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 2f927aa1e68d..730f929eb4c2 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -19,6 +19,7 @@ import ( cliflags "github.com/docker/cli/cli/flags" "github.com/docker/cli/cli/version" platformsignals "github.com/docker/cli/cmd/docker/internal/signals" + "github.com/docker/cli/internal" "github.com/docker/docker/api/types/versions" "github.com/docker/docker/errdefs" "github.com/pkg/errors" @@ -51,14 +52,14 @@ func dockerMain(ctx context.Context) error { } // getExitCode returns the exit-code to use for the given error. -// If err is a [cli.StatusError] and has a StatusCode set, it uses the +// If err is a [internal.StatusError] and has a StatusCode set, it uses the // status-code from it, otherwise it returns "1" for any error. func getExitCode(err error) int { if err == nil { return 0 } - var stErr cli.StatusError - if errors.As(err, &stErr) && stErr.StatusCode != 0 { // FIXME(thaJeztah): StatusCode should never be used with a zero status-code. Check if we do this anywhere. + var stErr internal.StatusError + if errors.As(err, &stErr) { return stErr.StatusCode } @@ -321,7 +322,7 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { statusCode = ws.ExitStatus() } - return cli.StatusError{ + return internal.StatusError{ StatusCode: statusCode, } } diff --git a/internal/status_error.go b/internal/status_error.go new file mode 100644 index 000000000000..7386033719a5 --- /dev/null +++ b/internal/status_error.go @@ -0,0 +1,36 @@ +package internal + +import "strconv" + +// StatusError reports an unsuccessful exit by a command. +// StatusCode must be non-zero. +// Cause may be nil if a generic error-message is desired. +type StatusError struct { + // Cause is the underlying error. + // It may be nil to generate a generic error message. + Cause error + // StatusCode is the exit status code. + // It must be non-zero. + StatusCode int +} + +// Error formats the error for printing. If a custom Status is provided, +// it is returned as-is, otherwise it generates a generic error-message +// based on the StatusCode. +func (e StatusError) Error() string { + if e.Cause == nil { + return "exit status " + strconv.Itoa(e.StatusCode) + } + return e.Cause.Error() +} + +// Unwrap returns the wrapped error. +// +// This allows StatusError to be checked with errors.Is. +func (e StatusError) Unwrap() error { + return e.Cause +} + +func (e StatusError) GetStatusCode() int { + return e.StatusCode +} From b1f899cf9f17657f535eca637cf5ca8770f18de7 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:59:06 +0100 Subject: [PATCH 2/2] cli/error: prevent breaking cli.StatusError for plugins This patch ensures that plugins can still use `cli.StatusError` and won't need to modify anything in the short term. In the longer term we should deprecate `cli.StatusError{}.Status` and instead only have `cli.StatusError{}.Cause`. A common interface is introduced for both `cli.StatusError` and `internal.StatusError` so that we can still use `errors.As` no matter which error is returned. Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli-plugins/plugin/plugin.go | 5 +-- cli/error.go | 73 +++++++++++++++++++++++++++++++++--- cli/error_test.go | 42 +++++++++++++++++++++ cmd/docker/docker.go | 4 +- internal/status_error.go | 3 +- 5 files changed, 116 insertions(+), 11 deletions(-) create mode 100644 cli/error_test.go diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index a4c1d6363129..5eea3ec67bcd 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -14,7 +14,6 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/connhelper" "github.com/docker/cli/cli/debug" - "github.com/docker/cli/internal" "github.com/docker/docker/client" "github.com/spf13/cobra" "go.opentelemetry.io/otel" @@ -94,12 +93,12 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { plugin := makeCmd(dockerCli) if err := RunPlugin(dockerCli, plugin, meta); err != nil { - var stErr internal.StatusError + var stErr cli.StatusCodeError if errors.As(err, &stErr) { _, _ = fmt.Fprintln(dockerCli.Err(), stErr) // StatusError should only be used for errors, and all errors should // have a non-zero exit status, so never exit with 0 - os.Exit(stErr.StatusCode) + os.Exit(stErr.GetStatusCode()) } _, _ = fmt.Fprintln(dockerCli.Err(), err) os.Exit(1) diff --git a/cli/error.go b/cli/error.go index 73ed2edf08d4..b1a7516d3843 100644 --- a/cli/error.go +++ b/cli/error.go @@ -4,9 +4,21 @@ import ( "github.com/docker/cli/internal" ) -// StatusError reports an unsuccessful exit by a command. -type StatusError interface { - Error() string +// StatusCodeError is a custom error type that reports an unsuccessful +// exit by a command. +// +// It is preferable to use this interface when seeking the status code +// of an error returned by the CLI. +// +// var statusCodeError cli.StatusCodeError +// err := someFunction() +// errors.As(err, &statusCodeError) +// fmt.Println(statusCodeError.GetStatusCode()) +// +// Internal to the CLI can use the [internal.StatusError] +// implementation directly. +type StatusCodeError interface { + error Unwrap() error // GetStatusCode returns the status code of the error. @@ -14,7 +26,58 @@ type StatusError interface { GetStatusCode() int } -// Pin the exported StatusError interface to the internal.StatusError type. +// StatusError implements [cli.StatusCodeError] and reports +// an unsuccessful exit by a command. +// +// StatusCode must be non-zero. +// Status/Cause may be empty/nil if a generic error-message is desired. +// +// Note: This error type is used by CLI plugins to report +// the exit code of a command and is thus discouraged from being +// modified. +// +// It is usually discouraged to use this error type directly as the +// [cli.StatusCodeError] interface should be used instead. +type StatusError struct { + // Deprecated: StatusError.Status is deprecated and should not be + // used. Instead, use StatusError.Cause. + Status string + + // Cause is the underlying error that caused the failure. It may be + // set to nil if a generic error-message is desired. + Cause error + + // StatusCode is the exit code of the command. This field must + // be non-zero. + StatusCode int +} + +// Error formats the error for printing. If a custom Status/Cause +// is provided, it is returned as-is, otherwise it generates a generic +// error-message based on the StatusCode. +func (e StatusError) Error() string { + if e.Status != "" { + return e.Status + } + return internal.StatusError{ + Cause: e.Cause, + StatusCode: e.StatusCode, + }.Error() +} + +func (e StatusError) Unwrap() error { + return e.Cause +} + +func (e StatusError) GetStatusCode() int { + return e.StatusCode +} + +// Pin the exported StatusCodeError interface to the +// [internal.StatusError] type. // This is necessary to ensure that the internal.StatusError type does // not break the compatibility of the exported interface. -var _ StatusError = internal.StatusError{} +var ( + _ StatusCodeError = internal.StatusError{} + _ StatusCodeError = StatusError{} +) diff --git a/cli/error_test.go b/cli/error_test.go new file mode 100644 index 000000000000..3203b2e7d006 --- /dev/null +++ b/cli/error_test.go @@ -0,0 +1,42 @@ +package cli + +import ( + "errors" + "testing" + + "github.com/docker/cli/internal" + "gotest.tools/v3/assert" +) + +func TestStatusError(t *testing.T) { + t.Run("custom status should be returned as-is", func(t *testing.T) { + statusError := StatusError{ + Status: "status", + StatusCode: 1, + } + assert.Equal(t, statusError.Error(), "status") + }) + + t.Run("cause error should be returned as-is", func(t *testing.T) { + statusError := StatusError{ + Cause: errors.New("cause"), + StatusCode: 1, + } + assert.Equal(t, statusError.Error(), "cause") + }) + + t.Run("generic error-message should be generated based on the StatusCode", func(t *testing.T) { + statusError := StatusError{ + StatusCode: 1, + } + assert.Equal(t, statusError.Error(), "exit status 1") + }) + + t.Run("exported StatusCodeError interface should be pinned to internal.StatusError type", func(t *testing.T) { + internalStatusError := internal.StatusError{} + statusError := StatusError{} + var e StatusCodeError + assert.Check(t, errors.As(internalStatusError, &e)) + assert.Check(t, errors.As(statusError, &e)) + }) +} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 730f929eb4c2..772daf3185fa 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -58,9 +58,9 @@ func getExitCode(err error) int { if err == nil { return 0 } - var stErr internal.StatusError + var stErr cli.StatusCodeError if errors.As(err, &stErr) { - return stErr.StatusCode + return stErr.GetStatusCode() } // No status-code provided; all errors should have a non-zero exit code. diff --git a/internal/status_error.go b/internal/status_error.go index 7386033719a5..c898b6817f18 100644 --- a/internal/status_error.go +++ b/internal/status_error.go @@ -2,7 +2,8 @@ package internal import "strconv" -// StatusError reports an unsuccessful exit by a command. +// StatusError implements [cli.StatusCodeError] and +// reports an unsuccessful exit by a command. // StatusCode must be non-zero. // Cause may be nil if a generic error-message is desired. type StatusError struct {