cli/command/container: remove reportError, and put StatusError to use

The `reportError` utility was present because cli.StatusError would print
the error decorated with `Status: <error-message>, Code: <exit-code>`.
That was not desirable in many cases as it would mess-up the output. To
prevent this, the CLI had code to check for an empty `Status` (error message)
in which case the error would be "ignored" (and only used for the exit-status),
and the `reportError` utility would be used to manually print a custom error
message before returning the error.

Now that bca2090 fixed the output format
of `cli.StatusError`, and 3dd6fc3 and
350a0b6 no longer discard these error,
we can get rid of this utility, and just set the error-message for
the status-error.

This patch:

- Introduces a `withHelp` which takes care of decorating errors with
  a "Run --help" hint for the user.
- Introduces a `toStatusError` utility that detects certain errors in
  the container to assign a corresponding exit-code (these error-codes
  can be used to distinguish "client" errors from "container" errors).
- Removes the `reportError` utility, and removes code that manually
  printed errors before returning.

Behavior is mostly unmodified, with the exception of some slight reformatting
of the errors:

- `withHelp` adds a `docker:` prefix to the error, to indicate the error
  is produced by the `docker` command. This prefix was already present
  in most cases.
- The "--help" hint is slightly updated ("Run 'docker run --help' for
  more information" instead of "See 'docker run --help'"), to make it
  more clear that it's a "call to action".
- An empty is added before the "--help" hint to separate it better from
  the error-message.

Before this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never".
    See 'docker run --help'.
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.
    $ echo $?
    127

With this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never"

    Run 'docker run --help' for more information
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.

    Run 'docker run --help' for more information
    $ echo $?
    127

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Author: thaJeztah

cli/command/container/create.go

@@ -92,8 +92,10 @@ 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 {
-		reportError(dockerCli.Err(), "create", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "create").Error(),
+			StatusCode: 125,
+		}
 	}
 	proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
 	newEnv := []string{}
@@ -107,12 +109,16 @@ 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 {
-		reportError(dockerCli.Err(), "create", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "create").Error(),
+			StatusCode: 125,
+		}
 	}
 	if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil {
-		reportError(dockerCli.Err(), "create", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "create").Error(),
+			StatusCode: 125,
+		}
 	}
 	id, err := createContainer(ctx, dockerCli, containerCfg, options)
 	if err != nil {

cli/command/container/create_test.go

@@ -189,8 +189,8 @@ func TestCreateContainerImagePullPolicyInvalid(t *testing.T) {
 
 			statusErr := cli.StatusError{}
 			assert.Check(t, errors.As(err, &statusErr))
-			assert.Equal(t, statusErr.StatusCode, 125)
-			assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg))
+			assert.Check(t, is.Equal(statusErr.StatusCode, 125))
+			assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg))
 		})
 	}
 }
@@ -285,7 +285,7 @@ func TestNewCreateCommandWithWarnings(t *testing.T) {
 	for _, tc := range testCases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
-			cli := test.NewFakeCli(&fakeClient{
+			fakeCLI := test.NewFakeCli(&fakeClient{
 				createContainerFunc: func(config *container.Config,
 					hostConfig *container.HostConfig,
 					networkingConfig *network.NetworkingConfig,
@@ -295,15 +295,15 @@ func TestNewCreateCommandWithWarnings(t *testing.T) {
 					return container.CreateResponse{}, nil
 				},
 			})
-			cmd := NewCreateCommand(cli)
+			cmd := NewCreateCommand(fakeCLI)
 			cmd.SetOut(io.Discard)
 			cmd.SetArgs(tc.args)
 			err := cmd.Execute()
 			assert.NilError(t, err)
 			if tc.warning {
-				golden.Assert(t, cli.ErrBuffer().String(), tc.name+".golden")
+				golden.Assert(t, fakeCLI.ErrBuffer().String(), tc.name+".golden")
 			} else {
-				assert.Equal(t, cli.ErrBuffer().String(), "")
+				assert.Equal(t, fakeCLI.ErrBuffer().String(), "")
 			}
 		})
 	}

cli/command/container/run.go

@@ -83,8 +83,10 @@ 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 {
-		reportError(dockerCli.Err(), "run", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "run").Error(),
+			StatusCode: 125,
+		}
 	}
 	proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
 	newEnv := []string{}
@@ -99,12 +101,16 @@ 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 {
-		reportError(dockerCli.Err(), "run", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "run").Error(),
+			StatusCode: 125,
+		}
 	}
 	if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil {
-		reportError(dockerCli.Err(), "run", err.Error(), true)
-		return cli.StatusError{StatusCode: 125}
+		return cli.StatusError{
+			Status:     withHelp(err, "run").Error(),
+			StatusCode: 125,
+		}
 	}
 	return runContainer(ctx, dockerCli, ropts, copts, containerCfg)
 }
@@ -139,8 +145,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 
 	containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions)
 	if err != nil {
-		reportError(stderr, "run", err.Error(), true)
-		return runStartContainerErr(err)
+		return toStatusError(err)
 	}
 	if runOpts.sigProxy {
 		sigc := notifyAllSignals()
@@ -204,12 +209,11 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 			<-errCh
 		}
 
-		reportError(stderr, "run", err.Error(), false)
 		if copts.autoRemove {
 			// wait container to be removed
 			<-statusChan
 		}
-		return runStartContainerErr(err)
+		return toStatusError(err)
 	}
 
 	if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
@@ -292,30 +296,40 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, containerID str
 	return resp.Close, nil
 }
 
-// reportError is a utility method that prints a user-friendly message
-// containing the error that occurred during parsing and a suggestion to get help
-func reportError(stderr io.Writer, name string, str string, withHelp bool) {
-	str = strings.TrimSuffix(str, ".") + "."
-	if withHelp {
-		str += "\nSee 'docker " + name + " --help'."
-	}
-	_, _ = fmt.Fprintln(stderr, "docker:", str)
+// withHelp decorates the error with a suggestion to use "--help".
+func withHelp(err error, commandName string) error {
+	return fmt.Errorf("docker: %w\n\nRun 'docker %s --help' for more information", err, commandName)
 }
 
-// if container start fails with 'not found'/'no such' error, return 127
-// if container start fails with 'permission denied' error, return 126
-// return 125 for generic docker daemon failures
-func runStartContainerErr(err error) error {
-	trimmedErr := strings.TrimPrefix(err.Error(), "Error response from daemon: ")
-	statusError := cli.StatusError{StatusCode: 125}
-	if strings.Contains(trimmedErr, "executable file not found") ||
-		strings.Contains(trimmedErr, "no such file or directory") ||
-		strings.Contains(trimmedErr, "system cannot find the file specified") {
-		statusError = cli.StatusError{StatusCode: 127}
-	} else if strings.Contains(trimmedErr, syscall.EACCES.Error()) ||
-		strings.Contains(trimmedErr, syscall.EISDIR.Error()) {
-		statusError = cli.StatusError{StatusCode: 126}
+// 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
+// error message and the Status field set as follows:
+//
+// - 125: for generic failures sent back from the daemon
+// - 126: if container start fails with 'permission denied' error
+// - 127: if container start fails with 'not found'/'no such' error
+func toStatusError(err error) error {
+	// TODO(thaJeztah): some of these errors originate from the container: should we actually suggest "--help" for those?
+
+	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(),
+			StatusCode: 127,
+		}
 	}
 
-	return statusError
+	if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) {
+		return cli.StatusError{
+			Status:     withHelp(err, "run").Error(),
+			StatusCode: 126,
+		}
+	}
+
+	return cli.StatusError{
+		Status:     withHelp(err, "run").Error(),
+		StatusCode: 125,
+	}
 }

cli/command/container/run_test.go

@@ -145,8 +145,10 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) {
 			cmd.SetOut(io.Discard)
 			cmd.SetErr(io.Discard)
 			err := cmd.Execute()
-			assert.Assert(t, err != nil)
-			assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
+			statusErr := cli.StatusError{}
+			assert.Check(t, errors.As(err, &statusErr))
+			assert.Check(t, is.Equal(statusErr.StatusCode, 125))
+			assert.Check(t, is.ErrorContains(err, tc.expectedError))
 		})
 	}
 }
@@ -179,8 +181,8 @@ func TestRunContainerImagePullPolicyInvalid(t *testing.T) {
 
 			statusErr := cli.StatusError{}
 			assert.Check(t, errors.As(err, &statusErr))
-			assert.Equal(t, statusErr.StatusCode, 125)
-			assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg))
+			assert.Check(t, is.Equal(statusErr.StatusCode, 125))
+			assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg))
 		})
 	}
 }