Skip to content

Commit

Permalink
cli/command: ctx cancel should not print or produce a non zero exit code
Browse files Browse the repository at this point in the history
The user might kill the CLI through a SIGINT/SIGTERM
which cancels the main context we pass around.

Currently the context cancel error is printed
alongside any other wrapped error with a generic
exit code (125).

This patch improves on this behavior and prevents
any error from being printed when they match
`context.Cancelled`.

The `cli.StatusError` error would wrap errors but
not provide a way to unwrap them. This would lead
to situations where `errors.Is` would not match the
underlying error.

Signed-off-by: Alano Terblanche <[email protected]>
  • Loading branch information
Benehiko committed Dec 5, 2024
1 parent 5f22178 commit a20d0c8
Show file tree
Hide file tree
Showing 14 changed files with 34 additions and 27 deletions.
2 changes: 1 addition & 1 deletion cli/cobra.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error {
}

return StatusError{
Status: fmt.Sprintf("%s\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()),
Cause: fmt.Errorf("%w\n\nUsage: %s\n\nRun '%s --help' for more information", err, cmd.UseLine(), cmd.CommandPath()),
StatusCode: 125,
}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/command/config/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,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 cli.StatusError{StatusCode: 1, Cause: err}
}
return nil
}
6 changes: 3 additions & 3 deletions cli/command/container/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ 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(),
Cause: withHelp(err, "create"),
StatusCode: 125,
}
}
Expand All @@ -110,13 +110,13 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet,
containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
if err != nil {
return cli.StatusError{
Status: withHelp(err, "create").Error(),
Cause: withHelp(err, "create"),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil {
return cli.StatusError{
Status: withHelp(err, "create").Error(),
Cause: withHelp(err, "create"),
StatusCode: 125,
}
}
Expand Down
12 changes: 6 additions & 6 deletions cli/command/container/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ 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(),
Cause: withHelp(err, "run"),
StatusCode: 125,
}
}
Expand All @@ -103,13 +103,13 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro
// just in case the parse does not exit
if err != nil {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
Cause: withHelp(err, "run"),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
Cause: withHelp(err, "run"),
StatusCode: 125,
}
}
Expand Down Expand Up @@ -315,20 +315,20 @@ func toStatusError(err error) 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(),
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(),
Cause: withHelp(err, "run"),
StatusCode: 126,
}
}

return cli.StatusError{
Status: withHelp(err, "run").Error(),
Cause: withHelp(err, "run"),
StatusCode: 125,
}
}
8 changes: 4 additions & 4 deletions cli/command/container/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,10 +294,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.(cli.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")
}
Expand Down
2 changes: 1 addition & 1 deletion cli/command/image/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,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 cli.StatusError{Cause: jerr, StatusCode: jerr.Code}
}
return err
}
Expand Down
4 changes: 2 additions & 2 deletions cli/command/inspect/inspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 cli.StatusError{StatusCode: 64, Cause: err}
}

var inspectErrs []string
Expand All @@ -90,7 +90,7 @@ func Inspect(out io.Writer, references []string, tmplStr string, getRef GetRefFu
if len(inspectErrs) != 0 {
return cli.StatusError{
StatusCode: 1,
Status: strings.Join(inspectErrs, "\n"),
Cause: errors.New(strings.Join(inspectErrs, "\n")),
}
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion cli/command/node/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,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 cli.StatusError{StatusCode: 1, Cause: err}
}
return nil
}
2 changes: 1 addition & 1 deletion cli/command/secret/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,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 cli.StatusError{StatusCode: 1, Cause: err}
}
return nil
}
2 changes: 1 addition & 1 deletion cli/command/service/inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,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 cli.StatusError{StatusCode: 1, Cause: err}
}
return nil
}
2 changes: 1 addition & 1 deletion cli/command/system/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func runEvents(ctx context.Context, dockerCli command.Cli, options *eventsOption
if err != nil {
return cli.StatusError{
StatusCode: 64,
Status: "Error parsing format: " + err.Error(),
Cause: fmt.Errorf("error parsing format: %w", err),
}
}
ctx, cancel := context.WithCancel(ctx)
Expand Down
2 changes: 1 addition & 1 deletion cli/command/system/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ func formatInfo(output io.Writer, info dockerInfo, format string) error {
if err != nil {
return cli.StatusError{
StatusCode: 64,
Status: "template parsing error: " + err.Error(),
Cause: fmt.Errorf("template parsing error: %w", err),
}
}
err = tmpl.Execute(output, info)
Expand Down
2 changes: 1 addition & 1 deletion cli/command/system/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,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 cli.StatusError{StatusCode: 64, Cause: err}
}

// TODO print error if kubernetes is used?
Expand Down
13 changes: 10 additions & 3 deletions cli/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,23 @@ import (

// StatusError reports an unsuccessful exit by a command.
type StatusError struct {
Status string
Cause error
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.Status == "" {
if e.Cause == nil {
return "exit status " + strconv.Itoa(e.StatusCode)
}
return e.Status
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
}

0 comments on commit a20d0c8

Please sign in to comment.