From 7c722c08d093c118ea727961be9aa0a23c83b733 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 12 Mar 2024 12:38:47 +0100 Subject: [PATCH] feat: standardize error for prompt Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> --- cli/command/builder/prune.go | 8 +- cli/command/builder/prune_test.go | 7 +- cli/command/container/prune.go | 8 +- cli/command/container/prune_test.go | 7 +- cli/command/image/prune.go | 8 +- cli/command/image/prune_test.go | 27 +-- cli/command/network/prune.go | 8 +- cli/command/network/prune_test.go | 7 +- cli/command/network/remove_test.go | 6 +- .../testdata/plugin-upgrade-terminate.golden | 2 +- cli/command/plugin/upgrade.go | 12 +- cli/command/plugin/upgrade_test.go | 7 +- cli/command/system/prune.go | 8 +- cli/command/system/prune_test.go | 10 +- cli/command/trust/revoke.go | 7 +- cli/command/trust/revoke_test.go | 25 +-- cli/command/trust/signer_remove.go | 6 +- .../trust-revoke-prompt-termination.golden | 1 - cli/command/utils.go | 7 +- cli/command/volume/prune.go | 12 +- cli/command/volume/prune_test.go | 8 +- .../volume/testdata/volume-prune-no.golden | 2 +- cmd/docker/docker.go | 4 + e2e/global/cli_test.go | 193 ++++++++++++++++++ e2e/plugin/trust_test.go | 60 +----- e2e/testutils/plugins.go | 102 +++++++++ .../basic => testutils/plugins}/basic.go | 5 + internal/test/cmd.go | 10 +- internal/test/writer.go | 23 +-- 29 files changed, 425 insertions(+), 165 deletions(-) create mode 100644 e2e/testutils/plugins.go rename e2e/{plugin/basic => testutils/plugins}/basic.go (90%) diff --git a/cli/command/builder/prune.go b/cli/command/builder/prune.go index cd453cbbc0..890a5204fe 100644 --- a/cli/command/builder/prune.go +++ b/cli/command/builder/prune.go @@ -2,6 +2,7 @@ package builder import ( "context" + "errors" "fmt" "strings" @@ -10,6 +11,7 @@ import ( "github.com/docker/cli/cli/command/completion" "github.com/docker/cli/opts" "github.com/docker/docker/api/types" + "github.com/docker/docker/errdefs" units "github.com/docker/go-units" "github.com/spf13/cobra" ) @@ -67,9 +69,13 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) warning = allCacheWarning } if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning); !r || err != nil { + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning) + if err != nil { return 0, "", err } + if !r { + return 0, "", errdefs.Cancelled(errors.New("`builder prune` has been cancelled")) + } } report, err := dockerCli.Client().BuildCachePrune(ctx, types.BuildCachePruneOptions{ diff --git a/cli/command/builder/prune_test.go b/cli/command/builder/prune_test.go index 57ea995445..e59e7ad788 100644 --- a/cli/command/builder/prune_test.go +++ b/cli/command/builder/prune_test.go @@ -5,10 +5,8 @@ import ( "errors" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" - "gotest.tools/v3/assert" ) func TestBuilderPromptTermination(t *testing.T) { @@ -21,8 +19,5 @@ func TestBuilderPromptTermination(t *testing.T) { }, }) cmd := NewPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/container/prune.go b/cli/command/container/prune.go index 583473fac7..62518cfee1 100644 --- a/cli/command/container/prune.go +++ b/cli/command/container/prune.go @@ -8,7 +8,9 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" "github.com/docker/cli/opts" + "github.com/docker/docker/errdefs" units "github.com/docker/go-units" + "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -54,9 +56,13 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) pruneFilters := command.PruneFilters(dockerCli, options.filter.Value()) if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning); !r || err != nil { + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning) + if err != nil { return 0, "", err } + if !r { + return 0, "", errdefs.Cancelled(errors.New("`container prune` has been cancelled")) + } } report, err := dockerCli.Client().ContainersPrune(ctx, pruneFilters) diff --git a/cli/command/container/prune_test.go b/cli/command/container/prune_test.go index 05cf87b37b..7cda89e612 100644 --- a/cli/command/container/prune_test.go +++ b/cli/command/container/prune_test.go @@ -4,12 +4,10 @@ import ( "context" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/pkg/errors" - "gotest.tools/v3/assert" ) func TestContainerPrunePromptTermination(t *testing.T) { @@ -22,8 +20,5 @@ func TestContainerPrunePromptTermination(t *testing.T) { }, }) cmd := NewPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/image/prune.go b/cli/command/image/prune.go index d627633cd5..9d666c9b5b 100644 --- a/cli/command/image/prune.go +++ b/cli/command/image/prune.go @@ -10,7 +10,9 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" "github.com/docker/cli/opts" + "github.com/docker/docker/errdefs" units "github.com/docker/go-units" + "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -68,9 +70,13 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) warning = allImageWarning } if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning); !r || err != nil { + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning) + if err != nil { return 0, "", err } + if !r { + return 0, "", errdefs.Cancelled(errors.New("`image prune` has been cancelled")) + } } report, err := dockerCli.Client().ImagesPrune(ctx, pruneFilters) diff --git a/cli/command/image/prune_test.go b/cli/command/image/prune_test.go index 472f9f7f6d..fa7b78b106 100644 --- a/cli/command/image/prune_test.go +++ b/cli/command/image/prune_test.go @@ -4,9 +4,10 @@ import ( "context" "fmt" "io" + "strings" "testing" - "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/streams" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" @@ -94,13 +95,18 @@ func TestNewPruneCommandSuccess(t *testing.T) { }, } for _, tc := range testCases { - cli := test.NewFakeCli(&fakeClient{imagesPruneFunc: tc.imagesPruneFunc}) - cmd := NewPruneCommand(cli) - cmd.SetOut(io.Discard) - cmd.SetArgs(tc.args) - err := cmd.Execute() - assert.NilError(t, err) - golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("prune-command-success.%s.golden", tc.name)) + t.Run(tc.name, func(t *testing.T) { + cli := test.NewFakeCli(&fakeClient{imagesPruneFunc: tc.imagesPruneFunc}) + // when prompted, answer "Y" to confirm the prune. + // will not be prompted if --force is used. + cli.SetIn(streams.NewIn(io.NopCloser(strings.NewReader("Y\n")))) + cmd := NewPruneCommand(cli) + cmd.SetOut(io.Discard) + cmd.SetArgs(tc.args) + err := cmd.Execute() + assert.NilError(t, err) + golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("prune-command-success.%s.golden", tc.name)) + }) } } @@ -114,8 +120,5 @@ func TestPrunePromptTermination(t *testing.T) { }, }) cmd := NewPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/network/prune.go b/cli/command/network/prune.go index 5f06770327..09c3cf32ea 100644 --- a/cli/command/network/prune.go +++ b/cli/command/network/prune.go @@ -7,6 +7,8 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/opts" + "github.com/docker/docker/errdefs" + "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -50,9 +52,13 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) pruneFilters := command.PruneFilters(dockerCli, options.filter.Value()) if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning); !r || err != nil { + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning) + if err != nil { return "", err } + if !r { + return "", errdefs.Cancelled(errors.New("`network prune` cancelled has been cancelled")) + } } report, err := dockerCli.Client().NetworksPrune(ctx, pruneFilters) diff --git a/cli/command/network/prune_test.go b/cli/command/network/prune_test.go index 9cf62a38e0..647899560d 100644 --- a/cli/command/network/prune_test.go +++ b/cli/command/network/prune_test.go @@ -4,12 +4,10 @@ import ( "context" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/pkg/errors" - "gotest.tools/v3/assert" ) func TestNetworkPrunePromptTermination(t *testing.T) { @@ -22,8 +20,5 @@ func TestNetworkPrunePromptTermination(t *testing.T) { }, }) cmd := NewPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/network/remove_test.go b/cli/command/network/remove_test.go index 5ad2a61df9..876cca89c2 100644 --- a/cli/command/network/remove_test.go +++ b/cli/command/network/remove_test.go @@ -5,7 +5,6 @@ import ( "io" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/errdefs" @@ -115,8 +114,5 @@ func TestNetworkRemovePromptTermination(t *testing.T) { }) cmd := newRemoveCommand(cli) cmd.SetArgs([]string{"existing-network"}) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/plugin/testdata/plugin-upgrade-terminate.golden b/cli/command/plugin/testdata/plugin-upgrade-terminate.golden index 511aad36cc..aaf7fd210a 100644 --- a/cli/command/plugin/testdata/plugin-upgrade-terminate.golden +++ b/cli/command/plugin/testdata/plugin-upgrade-terminate.golden @@ -1,2 +1,2 @@ Upgrading plugin foo/bar from localhost:5000/foo/bar:v0.1.0 to localhost:5000/foo/bar:v1.0.0 -Plugin images do not match, are you sure? [y/N] \ No newline at end of file +Plugin images do not match, are you sure? [y/N] diff --git a/cli/command/plugin/upgrade.go b/cli/command/plugin/upgrade.go index fc7abb4127..a3abff2116 100644 --- a/cli/command/plugin/upgrade.go +++ b/cli/command/plugin/upgrade.go @@ -8,6 +8,7 @@ import ( "github.com/distribution/reference" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/jsonmessage" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -63,11 +64,12 @@ func runUpgrade(ctx context.Context, dockerCli command.Cli, opts pluginOptions) fmt.Fprintf(dockerCli.Out(), "Upgrading plugin %s from %s to %s\n", p.Name, reference.FamiliarString(old), reference.FamiliarString(remote)) if !opts.skipRemoteCheck && remote.String() != old.String() { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), "Plugin images do not match, are you sure?"); !r || err != nil { - if err != nil { - return errors.Wrap(err, "canceling upgrade request") - } - return errors.New("canceling upgrade request") + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), "Plugin images do not match, are you sure?") + if err != nil { + return err + } + if !r { + return errdefs.Cancelled(errors.New("`plugin upgrade` has been cancelled")) } } diff --git a/cli/command/plugin/upgrade_test.go b/cli/command/plugin/upgrade_test.go index 6511e4364c..f4795efa66 100644 --- a/cli/command/plugin/upgrade_test.go +++ b/cli/command/plugin/upgrade_test.go @@ -5,11 +5,9 @@ import ( "io" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" "github.com/pkg/errors" - "gotest.tools/v3/assert" "gotest.tools/v3/golden" ) @@ -34,9 +32,6 @@ func TestUpgradePromptTermination(t *testing.T) { // need to set a remote address that does not match the plugin // reference sent by the `pluginInspectFunc` cmd.SetArgs([]string{"foo/bar", "localhost:5000/foo/bar:v1.0.0"}) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) golden.Assert(t, cli.OutBuffer().String(), "plugin-upgrade-terminate.golden") } diff --git a/cli/command/system/prune.go b/cli/command/system/prune.go index b3200ecfad..0c5dc8b50e 100644 --- a/cli/command/system/prune.go +++ b/cli/command/system/prune.go @@ -17,8 +17,10 @@ import ( "github.com/docker/cli/cli/command/volume" "github.com/docker/cli/opts" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" "github.com/docker/go-units" "github.com/fvbommel/sortorder" + "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -75,9 +77,13 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) return fmt.Errorf(`ERROR: The "until" filter is not supported with "--volumes"`) } if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), confirmationMessage(dockerCli, options)); !r || err != nil { + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), confirmationMessage(dockerCli, options)) + if err != nil { return err } + if !r { + return errdefs.Cancelled(errors.New("`system prune` has been cancelled")) + } } pruneFuncs := []func(ctx context.Context, dockerCli command.Cli, all bool, filter opts.FilterOpt) (uint64, string, error){ container.RunPrune, diff --git a/cli/command/system/prune_test.go b/cli/command/system/prune_test.go index 30cfb727a2..d895da8422 100644 --- a/cli/command/system/prune_test.go +++ b/cli/command/system/prune_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" @@ -18,7 +17,7 @@ func TestPrunePromptPre131DoesNotIncludeBuildCache(t *testing.T) { cli := test.NewFakeCli(&fakeClient{version: "1.30"}) cmd := newPruneCommand(cli) cmd.SetArgs([]string{}) - assert.NilError(t, cmd.Execute()) + assert.ErrorContains(t, cmd.Execute(), "`system prune` has been cancelled") expected := `WARNING! This will remove: - all stopped containers - all networks not used by at least one container @@ -36,7 +35,7 @@ func TestPrunePromptFilters(t *testing.T) { cmd := newPruneCommand(cli) cmd.SetArgs([]string{"--filter", "until=24h", "--filter", "label=hello-world", "--filter", "label!=foo=bar", "--filter", "label=bar=baz"}) - assert.NilError(t, cmd.Execute()) + assert.ErrorContains(t, cmd.Execute(), "`system prune` has been cancelled") expected := `WARNING! This will remove: - all stopped containers - all networks not used by at least one container @@ -69,8 +68,5 @@ func TestSystemPrunePromptTermination(t *testing.T) { }) cmd := newPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) + test.TerminatePrompt(ctx, t, cmd, cli) } diff --git a/cli/command/trust/revoke.go b/cli/command/trust/revoke.go index ac391a69c2..773231de18 100644 --- a/cli/command/trust/revoke.go +++ b/cli/command/trust/revoke.go @@ -8,6 +8,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/image" "github.com/docker/cli/cli/trust" + "github.com/docker/docker/errdefs" "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/theupdateframework/notary/client" @@ -45,12 +46,10 @@ func revokeTrust(ctx context.Context, dockerCLI command.Cli, remote string, opti if imgRefAndAuth.Tag() == "" && !options.forceYes { deleteRemote, err := command.PromptForConfirmation(ctx, dockerCLI.In(), dockerCLI.Out(), fmt.Sprintf("Please confirm you would like to delete all signature data for %s?", remote)) if err != nil { - fmt.Fprintf(dockerCLI.Out(), "\nAborting action.\n") - return errors.Wrap(err, "aborting action") + return err } if !deleteRemote { - fmt.Fprintf(dockerCLI.Out(), "\nAborting action.\n") - return nil + return errdefs.Cancelled(errors.New("`trust revoke` has been cancelled")) } } diff --git a/cli/command/trust/revoke_test.go b/cli/command/trust/revoke_test.go index a48e9898d5..c91e6845d0 100644 --- a/cli/command/trust/revoke_test.go +++ b/cli/command/trust/revoke_test.go @@ -5,7 +5,6 @@ import ( "io" "testing" - "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/trust" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/notary" @@ -58,6 +57,8 @@ func TestTrustRevokeCommandErrors(t *testing.T) { } func TestTrustRevokeCommand(t *testing.T) { + revokeCancelledError := "`trust revoke` has been cancelled" + testCases := []struct { doc string notaryRepository func(trust.ImageRefAndAuth, []string) (client.Repository, error) @@ -69,7 +70,8 @@ func TestTrustRevokeCommand(t *testing.T) { doc: "OfflineErrors_Confirm", notaryRepository: notary.GetOfflineNotaryRepository, args: []string{"reg-name.io/image"}, - expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] \nAborting action.", + expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] ", + expectedErr: revokeCancelledError, }, { doc: "OfflineErrors_Offline", @@ -87,7 +89,8 @@ func TestTrustRevokeCommand(t *testing.T) { doc: "UninitializedErrors_Confirm", notaryRepository: notary.GetUninitializedNotaryRepository, args: []string{"reg-name.io/image"}, - expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] \nAborting action.", + expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] ", + expectedErr: revokeCancelledError, }, { doc: "UninitializedErrors_NoTrustData", @@ -105,7 +108,8 @@ func TestTrustRevokeCommand(t *testing.T) { doc: "EmptyNotaryRepo_Confirm", notaryRepository: notary.GetEmptyTargetsNotaryRepository, args: []string{"reg-name.io/image"}, - expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] \nAborting action.", + expectedMessage: "Please confirm you would like to delete all signature data for reg-name.io/image? [y/N] ", + expectedErr: revokeCancelledError, }, { doc: "EmptyNotaryRepo_NoSignedTags", @@ -123,7 +127,8 @@ func TestTrustRevokeCommand(t *testing.T) { doc: "AllSigConfirmation", notaryRepository: notary.GetEmptyTargetsNotaryRepository, args: []string{"alpine"}, - expectedMessage: "Please confirm you would like to delete all signature data for alpine? [y/N] \nAborting action.", + expectedMessage: "Please confirm you would like to delete all signature data for alpine? [y/N] ", + expectedErr: revokeCancelledError, }, } @@ -136,9 +141,9 @@ func TestTrustRevokeCommand(t *testing.T) { cmd.SetOut(io.Discard) if tc.expectedErr != "" { assert.ErrorContains(t, cmd.Execute(), tc.expectedErr) - return + } else { + assert.NilError(t, cmd.Execute()) } - assert.NilError(t, cmd.Execute()) assert.Check(t, is.Contains(cli.OutBuffer().String(), tc.expectedMessage)) }) } @@ -159,10 +164,6 @@ func TestRevokeTrustPromptTermination(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cmd := newRevokeCommand(cli) cmd.SetArgs([]string{"example/trust-demo"}) - test.TerminatePrompt(ctx, t, cmd, cli, func(t *testing.T, err error) { - t.Helper() - assert.ErrorIs(t, err, command.ErrPromptTerminated) - }) - assert.Equal(t, cli.ErrBuffer().String(), "") + test.TerminatePrompt(ctx, t, cmd, cli) golden.Assert(t, cli.OutBuffer().String(), "trust-revoke-prompt-termination.golden") } diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index 06deaa7e0d..1c85fcd9fd 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -132,10 +132,12 @@ func removeSingleSigner(ctx context.Context, dockerCLI command.Cli, repoName, si } ok, err := maybePromptForSignerRemoval(ctx, dockerCLI, repoName, signerName, isLastSigner, forceYes) - if err != nil || !ok { - fmt.Fprintf(dockerCLI.Out(), "\nAborting action.\n") + if err != nil { return false, err } + if !ok { + return false, nil + } if err := notaryRepo.RemoveDelegationKeys(releasesRoleTUFName, role.KeyIDs); err != nil { return false, err diff --git a/cli/command/trust/testdata/trust-revoke-prompt-termination.golden b/cli/command/trust/testdata/trust-revoke-prompt-termination.golden index bf854da789..cceedd79d4 100644 --- a/cli/command/trust/testdata/trust-revoke-prompt-termination.golden +++ b/cli/command/trust/testdata/trust-revoke-prompt-termination.golden @@ -1,2 +1 @@ Please confirm you would like to delete all signature data for example/trust-demo? [y/N] -Aborting action. diff --git a/cli/command/utils.go b/cli/command/utils.go index df1f876faf..ae93dbc30a 100644 --- a/cli/command/utils.go +++ b/cli/command/utils.go @@ -19,6 +19,7 @@ import ( "github.com/docker/docker/api/types/filters" mounttypes "github.com/docker/docker/api/types/mount" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" "github.com/moby/sys/sequential" "github.com/pkg/errors" "github.com/spf13/pflag" @@ -75,9 +76,7 @@ func PrettyPrint(i any) string { } } -type PromptError error - -var ErrPromptTerminated = PromptError(errors.New("prompt terminated")) +var ErrPromptTerminated = errdefs.Cancelled(errors.New("prompt terminated")) // PromptForConfirmation requests and checks confirmation from the user. // This will display the provided message followed by ' [y/N] '. If the user @@ -123,6 +122,8 @@ func PromptForConfirmation(ctx context.Context, ins io.Reader, outs io.Writer, m select { case <-notifyCtx.Done(): + // print a newline on termination + fmt.Fprintln(outs, "") return false, ErrPromptTerminated case r := <-result: return r, nil diff --git a/cli/command/volume/prune.go b/cli/command/volume/prune.go index eff271665b..e1729f23c2 100644 --- a/cli/command/volume/prune.go +++ b/cli/command/volume/prune.go @@ -32,10 +32,6 @@ func NewPruneCommand(dockerCli command.Cli) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { spaceReclaimed, output, err := runPrune(cmd.Context(), dockerCli, options) if err != nil { - if errdefs.IsCancelled(err) { - fmt.Fprintln(dockerCli.Out(), output) - return nil - } return err } if output != "" { @@ -81,8 +77,12 @@ func runPrune(ctx context.Context, dockerCli command.Cli, options pruneOptions) warning = allVolumesWarning } if !options.force { - if r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning); !r || err != nil { - return 0, "", errdefs.Cancelled(errors.New("user cancelled operation")) + r, err := command.PromptForConfirmation(ctx, dockerCli.In(), dockerCli.Out(), warning) + if err != nil { + return 0, "", err + } + if !r { + return 0, "", errdefs.Cancelled(errors.New("`volume prune` has been cancelled")) } } diff --git a/cli/command/volume/prune_test.go b/cli/command/volume/prune_test.go index 03fbe9dcd0..da10ae20cc 100644 --- a/cli/command/volume/prune_test.go +++ b/cli/command/volume/prune_test.go @@ -155,6 +155,7 @@ func TestVolumePrunePromptYes(t *testing.T) { cli.SetIn(streams.NewIn(io.NopCloser(strings.NewReader(input)))) cmd := NewPruneCommand(cli) + cmd.SetArgs([]string{}) assert.NilError(t, cmd.Execute()) golden.Assert(t, cli.OutBuffer().String(), "volume-prune-yes.golden") } @@ -171,7 +172,8 @@ func TestVolumePrunePromptNo(t *testing.T) { cli.SetIn(streams.NewIn(io.NopCloser(strings.NewReader(input)))) cmd := NewPruneCommand(cli) - assert.NilError(t, cmd.Execute()) + cmd.SetArgs([]string{}) + assert.ErrorContains(t, cmd.Execute(), "`volume prune` has been cancelled") golden.Assert(t, cli.OutBuffer().String(), "volume-prune-no.golden") } } @@ -196,7 +198,7 @@ func TestVolumePrunePromptTerminate(t *testing.T) { }) cmd := NewPruneCommand(cli) - test.TerminatePrompt(ctx, t, cmd, cli, nil) - + cmd.SetArgs([]string{}) + test.TerminatePrompt(ctx, t, cmd, cli) golden.Assert(t, cli.OutBuffer().String(), "volume-prune-terminate.golden") } diff --git a/cli/command/volume/testdata/volume-prune-no.golden b/cli/command/volume/testdata/volume-prune-no.golden index 8e918f9c9e..9f7c8c76a5 100644 --- a/cli/command/volume/testdata/volume-prune-no.golden +++ b/cli/command/volume/testdata/volume-prune-no.golden @@ -1,2 +1,2 @@ WARNING! This will remove anonymous local volumes not used by at least one container. -Are you sure you want to continue? [y/N] +Are you sure you want to continue? [y/N] \ No newline at end of file diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index c79bfaaa0d..2c0a82b89c 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -17,6 +17,7 @@ import ( "github.com/docker/cli/cli/version" platformsignals "github.com/docker/cli/cmd/docker/internal/signals" "github.com/docker/docker/api/types/versions" + "github.com/docker/docker/errdefs" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -43,6 +44,9 @@ func main() { } os.Exit(sterr.StatusCode) } + if errdefs.IsCancelled(err) { + os.Exit(0) + } fmt.Fprintln(dockerCli.Err(), err) os.Exit(1) } diff --git a/e2e/global/cli_test.go b/e2e/global/cli_test.go index 863079b1c3..91d507eb70 100644 --- a/e2e/global/cli_test.go +++ b/e2e/global/cli_test.go @@ -1,14 +1,25 @@ package global import ( + "bufio" + "bytes" + "context" + "io" "net/http" "net/http/httptest" "strings" + "syscall" "testing" + "time" + "github.com/docker/cli/e2e/internal/fixtures" + "github.com/docker/cli/e2e/testutils" + "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/environment" + "github.com/docker/docker/api/types/versions" "gotest.tools/v3/assert" "gotest.tools/v3/icmd" + "gotest.tools/v3/poll" "gotest.tools/v3/skip" ) @@ -65,3 +76,185 @@ func TestTCPSchemeUsesHTTPProxyEnv(t *testing.T) { assert.Equal(t, strings.TrimSpace(result.Stdout()), "99.99.9") assert.Equal(t, received, "docker.acme.example.com:2376") } + +// Test that the prompt command exits with 0 +// when the user sends SIGINT/SIGTERM to the process +func TestPromptExitCode(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + dir := fixtures.SetupConfigFile(t) + t.Cleanup(dir.Remove) + + defaultCmdOpts := []icmd.CmdOp{ + fixtures.WithConfig(dir.Path()), + fixtures.WithNotary, + } + + testCases := []struct { + name string + run func(t *testing.T) icmd.Cmd + }{ + { + name: "volume prune", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "volume", "prune") + }, + }, + { + name: "network prune", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "network", "prune") + }, + }, + { + name: "container prune", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "container", "prune") + }, + }, + { + name: "image prune", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "image", "prune") + }, + }, + { + name: "system prune", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "system", "prune") + }, + }, + { + name: "revoke trust", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + return icmd.Command("docker", "trust", "revoke", "example/trust-demo") + }, + }, + { + name: "plugin install", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + skip.If(t, versions.LessThan(environment.DaemonAPIVersion(t), "1.44")) + + pluginDir := testutils.SetupPlugin(t, ctx) + t.Cleanup(pluginDir.Remove) + + plugin := "registry:5000/plugin-content-trust-install:latest" + + icmd.RunCommand("docker", "plugin", "create", plugin, pluginDir.Path()).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "push", plugin), defaultCmdOpts...).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "rm", "-f", plugin), defaultCmdOpts...).Assert(t, icmd.Success) + return icmd.Command("docker", "plugin", "install", plugin) + }, + }, + { + name: "plugin upgrade", + run: func(t *testing.T) icmd.Cmd { + t.Helper() + skip.If(t, versions.LessThan(environment.DaemonAPIVersion(t), "1.44")) + + pluginLatestDir := testutils.SetupPlugin(t, ctx) + t.Cleanup(pluginLatestDir.Remove) + pluginNextDir := testutils.SetupPlugin(t, ctx) + t.Cleanup(pluginNextDir.Remove) + + plugin := "registry:5000/plugin-content-trust-upgrade" + + icmd.RunCommand("docker", "plugin", "create", plugin+":latest", pluginLatestDir.Path()).Assert(t, icmd.Success) + icmd.RunCommand("docker", "plugin", "create", plugin+":next", pluginNextDir.Path()).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "push", plugin+":latest"), defaultCmdOpts...).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "push", plugin+":next"), defaultCmdOpts...).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "rm", "-f", plugin+":latest"), defaultCmdOpts...).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "rm", "-f", plugin+":next"), defaultCmdOpts...).Assert(t, icmd.Success) + icmd.RunCmd(icmd.Command("docker", "plugin", "install", "--disable", "--grant-all-permissions", plugin+":latest"), defaultCmdOpts...).Assert(t, icmd.Success) + return icmd.Command("docker", "plugin", "upgrade", plugin+":latest", plugin+":next") + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run("case="+tc.name, func(t *testing.T) { + t.Parallel() + + buf := new(bytes.Buffer) + bufioWriter := bufio.NewWriter(buf) + + writeDone := make(chan struct{}) + w := test.NewWriterWithHook(bufioWriter, func(p []byte) { + writeDone <- struct{}{} + }) + + drainChCtx, drainChCtxCancel := context.WithCancel(ctx) + t.Cleanup(drainChCtxCancel) + + drainChannel(drainChCtx, writeDone) + + r, _ := io.Pipe() + defer r.Close() + result := icmd.StartCmd(tc.run(t), + append(defaultCmdOpts, + icmd.WithStdout(w), + icmd.WithStderr(w), + icmd.WithStdin(r))...) + + poll.WaitOn(t, func(t poll.LogT) poll.Result { + select { + case <-ctx.Done(): + return poll.Error(ctx.Err()) + default: + + if err := bufioWriter.Flush(); err != nil { + return poll.Continue(err.Error()) + } + if strings.Contains(buf.String(), "[y/N]") { + return poll.Success() + } + + return poll.Continue("command did not prompt for confirmation, instead prompted:\n%s\n", buf.String()) + } + }, poll.WithDelay(100*time.Millisecond), poll.WithTimeout(1*time.Second)) + + drainChCtxCancel() + + assert.NilError(t, result.Cmd.Process.Signal(syscall.SIGINT)) + + proc, err := result.Cmd.Process.Wait() + assert.NilError(t, err) + assert.Equal(t, proc.ExitCode(), 0, "expected exit code to be 0, got %d", proc.ExitCode()) + + processCtx, processCtxCancel := context.WithTimeout(ctx, time.Second) + t.Cleanup(processCtxCancel) + + select { + case <-processCtx.Done(): + t.Fatal("timed out waiting for new line after process exit") + case <-writeDone: + buf.Reset() + assert.NilError(t, bufioWriter.Flush()) + assert.Equal(t, buf.String(), "\n", "expected a new line after the process exits from SIGINT") + } + }) + } +} + +func drainChannel(ctx context.Context, ch <-chan struct{}) { + go func() { + for { + select { + case <-ctx.Done(): + return + case <-ch: + } + } + }() +} diff --git a/e2e/plugin/trust_test.go b/e2e/plugin/trust_test.go index c6957d6f37..b32bfaf5b0 100644 --- a/e2e/plugin/trust_test.go +++ b/e2e/plugin/trust_test.go @@ -1,20 +1,14 @@ package plugin import ( - "encoding/json" + "context" "fmt" - "os" - "os/exec" - "path/filepath" "testing" "github.com/docker/cli/e2e/internal/fixtures" + "github.com/docker/cli/e2e/testutils" "github.com/docker/cli/internal/test/environment" - "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/versions" - "github.com/pkg/errors" - "gotest.tools/v3/assert" - "gotest.tools/v3/fs" "gotest.tools/v3/icmd" "gotest.tools/v3/skip" ) @@ -31,8 +25,11 @@ func TestInstallWithContentTrust(t *testing.T) { dir := fixtures.SetupConfigFile(t) defer dir.Remove() - pluginDir := preparePluginDir(t) - defer pluginDir.Remove() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + pluginDir := testutils.SetupPlugin(t, ctx) + t.Cleanup(pluginDir.Remove) icmd.RunCommand("docker", "plugin", "create", pluginName, pluginDir.Path()).Assert(t, icmd.Success) result := icmd.RunCmd(icmd.Command("docker", "plugin", "push", pluginName), @@ -73,46 +70,3 @@ func TestInstallWithContentTrustUntrusted(t *testing.T) { Err: "Error: remote trust data does not exist", }) } - -func preparePluginDir(t *testing.T) *fs.Dir { - t.Helper() - p := &types.PluginConfig{ - Interface: types.PluginConfigInterface{ - Socket: "basic.sock", - Types: []types.PluginInterfaceType{{Capability: "docker.dummy/1.0"}}, - }, - Entrypoint: []string{"/basic"}, - } - configJSON, err := json.Marshal(p) - assert.NilError(t, err) - - binPath, err := ensureBasicPluginBin() - assert.NilError(t, err) - - dir := fs.NewDir(t, "plugin_test", - fs.WithFile("config.json", string(configJSON), fs.WithMode(0o644)), - fs.WithDir("rootfs", fs.WithMode(0o755)), - ) - icmd.RunCommand("/bin/cp", binPath, dir.Join("rootfs", p.Entrypoint[0])).Assert(t, icmd.Success) - return dir -} - -func ensureBasicPluginBin() (string, error) { - name := "docker-basic-plugin" - p, err := exec.LookPath(name) - if err == nil { - return p, nil - } - - goBin, err := exec.LookPath("/usr/local/go/bin/go") - if err != nil { - return "", err - } - installPath := filepath.Join(os.Getenv("GOPATH"), "bin", name) - cmd := exec.Command(goBin, "build", "-o", installPath, "./basic") - cmd.Env = append(os.Environ(), "CGO_ENABLED=0") - if out, err := cmd.CombinedOutput(); err != nil { - return "", errors.Wrapf(err, "error building basic plugin bin: %s", string(out)) - } - return installPath, nil -} diff --git a/e2e/testutils/plugins.go b/e2e/testutils/plugins.go new file mode 100644 index 0000000000..e194eef9a5 --- /dev/null +++ b/e2e/testutils/plugins.go @@ -0,0 +1,102 @@ +package testutils + +import ( + "context" + "crypto/rand" + "embed" + "encoding/base64" + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/docker/docker/api/types" + "github.com/pkg/errors" + "gotest.tools/v3/assert" + "gotest.tools/v3/fs" + "gotest.tools/v3/icmd" +) + +//go:embed plugins/* +var plugins embed.FS + +// SetupPlugin builds a plugin and creates a temporary +// directory with the plugin's config.json and rootfs. +func SetupPlugin(t *testing.T, ctx context.Context) *fs.Dir { + t.Helper() + + p := &types.PluginConfig{ + Linux: types.PluginConfigLinux{ + Capabilities: []string{"CAP_SYS_ADMIN"}, + }, + Interface: types.PluginConfigInterface{ + Socket: "basic.sock", + Types: []types.PluginInterfaceType{{Capability: "docker.dummy/1.0"}}, + }, + Entrypoint: []string{"/basic"}, + } + configJSON, err := json.Marshal(p) + assert.NilError(t, err) + + binPath, err := buildPlugin(t, ctx) + assert.NilError(t, err) + + dir := fs.NewDir(t, "plugin_test", + fs.WithFile("config.json", string(configJSON), fs.WithMode(0o644)), + fs.WithDir("rootfs", fs.WithMode(0o755)), + ) + + icmd.RunCommand("/bin/cp", binPath, dir.Join("rootfs", p.Entrypoint[0])).Assert(t, icmd.Success) + return dir +} + +// buildPlugin uses Go to build a plugin from one of the source files in the plugins directory. +// It returns the path to the built plugin binary. +// To allow for multiple plugins to be built in parallel, the plugin is compiled with a unique +// identifier in the binary. This is done by setting a linker flag with the -ldflags option. +func buildPlugin(t *testing.T, ctx context.Context) (string, error) { + t.Helper() + + randomName, err := randomString() + if err != nil { + return "", err + } + + goBin, err := exec.LookPath("/usr/local/go/bin/go") + if err != nil { + return "", err + } + installPath := filepath.Join(os.Getenv("GOPATH"), "bin", randomName) + + pluginContent, err := plugins.ReadFile("plugins/basic.go") + if err != nil { + return "", err + } + dir := fs.NewDir(t, "plugin_build") + if err := os.WriteFile(dir.Join("main.go"), pluginContent, 0o644); err != nil { + return "", err + } + defer dir.Remove() + + cmd := exec.CommandContext(ctx, goBin, "build", "-ldflags", + fmt.Sprintf("-X 'main.UNIQUEME=%s'", randomName), + "-o", installPath, dir.Join("main.go")) + + cmd.Env = append(os.Environ(), "CGO_ENABLED=0") + + if out, err := cmd.CombinedOutput(); err != nil { + return "", errors.Wrapf(err, "error building basic plugin bin: %s", string(out)) + } + + return installPath, nil +} + +func randomString() (string, error) { + b := make([]byte, 8) + if _, err := rand.Read(b); err != nil { + return "", err + } + return base64.StdEncoding.EncodeToString(b), nil +} diff --git a/e2e/plugin/basic/basic.go b/e2e/testutils/plugins/basic.go similarity index 90% rename from e2e/plugin/basic/basic.go rename to e2e/testutils/plugins/basic.go index dbff191030..00a402baa8 100644 --- a/e2e/plugin/basic/basic.go +++ b/e2e/testutils/plugins/basic.go @@ -9,6 +9,11 @@ import ( "time" ) +// set by the compile flags to get around content sha being the same +var ( + UNIQUEME string +) + func main() { p, err := filepath.Abs(filepath.Join("run", "docker", "plugins")) if err != nil { diff --git a/internal/test/cmd.go b/internal/test/cmd.go index 562542e6f8..04df496a83 100644 --- a/internal/test/cmd.go +++ b/internal/test/cmd.go @@ -7,12 +7,13 @@ import ( "testing" "time" + "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/streams" "github.com/spf13/cobra" "gotest.tools/v3/assert" ) -func TerminatePrompt(ctx context.Context, t *testing.T, cmd *cobra.Command, cli *FakeCli, assertFunc func(*testing.T, error)) { +func TerminatePrompt(ctx context.Context, t *testing.T, cmd *cobra.Command, cli *FakeCli) { t.Helper() errChan := make(chan error) @@ -73,11 +74,6 @@ func TerminatePrompt(ctx context.Context, t *testing.T, cmd *cobra.Command, cli t.Logf("command stderr:\n%s\n", cli.ErrBuffer().String()) t.Fatalf("command %s did not return after SIGINT", cmd.Name()) case err := <-errChan: - if assertFunc != nil { - assertFunc(t, err) - } else { - assert.NilError(t, err) - assert.Equal(t, cli.ErrBuffer().String(), "") - } + assert.ErrorIs(t, err, command.ErrPromptTerminated) } } diff --git a/internal/test/writer.go b/internal/test/writer.go index c4ceede21e..3d922fb57a 100644 --- a/internal/test/writer.go +++ b/internal/test/writer.go @@ -4,27 +4,22 @@ import ( "io" ) -// WriterWithHook is an io.Writer that calls a hook function -// after every write. -// This is useful in testing to wait for a write to complete, -// or to check what was written. -// To create a WriterWithHook use the NewWriterWithHook function. -type WriterWithHook struct { +type writerWithHook struct { actualWriter io.Writer hook func([]byte) } -// Write writes p to the actual writer and then calls the hook function. -func (w *WriterWithHook) Write(p []byte) (n int, err error) { +func (w *writerWithHook) Write(p []byte) (n int, err error) { defer w.hook(p) return w.actualWriter.Write(p) } -var _ io.Writer = (*WriterWithHook)(nil) +var _ io.Writer = (*writerWithHook)(nil) -// NewWriterWithHook returns a new WriterWithHook that still writes to the actualWriter -// but also calls the hook function after every write. -// The hook function is useful for testing, or waiting for a write to complete. -func NewWriterWithHook(actualWriter io.Writer, hook func([]byte)) *WriterWithHook { - return &WriterWithHook{actualWriter: actualWriter, hook: hook} +// NewWriterWithHook returns a io.Writer that still +// writes to the actualWriter but also calls the hook function +// after every write. It is useful to use this function when +// you need to wait for a writer to complete writing inside a test. +func NewWriterWithHook(actualWriter io.Writer, hook func([]byte)) *writerWithHook { + return &writerWithHook{actualWriter: actualWriter, hook: hook} }