feat: standardize error for prompt

[Benehiko]

Follow up to #4888

This PR standardizes the commands return value that use PromptForConfirmation and verifies the exit code to be 0 on prompt termination.

- What I did
Add e2e tests, update command prompt termination tests and adjust error type returned from PromptForConfirmation function.

- How I did it

- How to verify it

- Description for the changelog

Standardize the exit code to 0 for cancelling a confirmation prompt with SIGINT.

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

Merging #4939 (7c722c0) into master (b8d5454) will decrease coverage by 0.04%.
Report is 8 commits behind head on master.
The diff coverage is 65.78%.

❗ Current head 7c722c0 differs from pull request most recent head 66e73bc. Consider uploading reports for the commit 66e73bc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4939      +/-   ##
==========================================
- Coverage   61.19%   61.15%   -0.04%     
==========================================
  Files         294      294              
  Lines       20538    20556      +18     
==========================================
+ Hits        12568    12571       +3     
- Misses       7076     7088      +12     
- Partials      894      897       +3     

[Benehiko]

Getting this test failure now https://github.com/docker/cli/actions/runs/8362281837/job/22892487855?pr=4939#step:4:397

Will look at it tomorrow. Most likely related #4948

[Benehiko]

--- FAIL: TestConnectAndWait (0.02s)
    --- FAIL: TestConnectAndWait/connect_goroutine_exits_after_EOF (0.02s)
        socket_test.go:182: timeout hit after 10ms: waiting for connect goroutine to exit
FAIL

https://github.com/docker/cli/actions/runs/8390226667/job/22978038463?pr=4939#step:5:81

🤔

Wonder if some flakiness has been introduced in this commit d68cc0e#diff-2b3ea721470acb354ca37a32dc40b7c0ae55bd43397a084adbd922fca5964e20

[krissetto]

overall LGTM after some minor nits and what has already been pointed out by the others

[laurazard]

LGTM

[Benehiko]

[laurazard]

@thaJeztah care to take a final look at this?

[Benehiko]

Could we merge this one?

[thaJeztah]

Thanks! Overall looks good; had a question about that magic UNIQUEME variable, and noticed some back-tics in the errors.

Honestly (but not for this PR), looking at PromptForConfirmation, I'm slightly wondering if it's doing too much

cli/cli/command/utils.go

Lines 92 to 98 in 155dc5e

	func PromptForConfirmation(ctx context.Context, ins io.Reader, outs io.Writer, message string) (bool, error) {
	if message == "" {
	message = "Are you sure you want to proceed?"
	}
	message += " [y/N] "
	_, _ = fmt.Fprint(outs, message)

Wondering if;

• haven't checked, but wondered if there's cases where No is not the default (so if it should take a default value), but maybe that's not the case.
• ☝️ that said, the [y/N] (capital N) indicates that N is always considered the default; so even if a caller would want to use a different default, the function appending the [y/N] means that there's not really a choice for them.
• we should remove the default message (looks like the only place where we pass an empty string is in a test)
• was even considering if we need the message at all, because all it does is print the message (unconditionally), and if that should be left to the caller;
• from that perspective I was looking at the outs io.Writer and if we need it, but that one looks more tricky, as we still want to print the newline 😓

[thaJeztah]

This one doesn't seem to be used anywhere, so could lead to a linter starting to flag it.

Possibly we could avoid that through something silly like _ = UNIQUEME, but slightly curious what the issue was that you ran into; did this cause issues in tests where Go was caching things? Or was there something else?

[Benehiko]

The e2e tests build a plugin and pushes that to a registry run inside docker (spun up by the tests). That said, for multiple tests doing docker plugin create and docker plugin push you would end up with sha collisions since the source code of the plugin binary built would be the same. The UNIQUEME variable is a way around this, so we can re-build the same plugin binary from the same source code generating a new sha hash.

See these tests for reference

cli/e2e/global/cli_test.go

Lines 143 to 181 in 7c722c0

	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")
	},
	},

[thaJeztah]

Oh! And now curious; wouldn't that be something we could take advantage of? If we're building (and pushing) the exact same plugin multiple times; would we be able to use that so that we don't have to? 🤔

Wondering now; ISTR we have something similar in moby/moby, where we need to have an image that's built and pushed in CI (but have something to verify if it needs to be built / built only once). I'd have to go look for that though 🙈

[Benehiko]

Not necessarily since there is also a test that verifies if it can push the plugin. Although optimization on if the plugin exists or not could be done, I didn't want to scope creep standardizing the plugin setup code for very small gains.

https://github.com/docker/cli/blob/master/e2e/plugin/trust_test.go#L37-L46

[thaJeztah]

Yes, definitely fine to look at separately

[Benehiko]

Thanks! Overall looks good; had a question about that magic UNIQUEME variable, and noticed some back-tics in the errors.

Honestly (but not for this PR), looking at PromptForConfirmation, I'm slightly wondering if it's doing too much

cli/cli/command/utils.go

Lines 92 to 98 in 155dc5e

	func PromptForConfirmation(ctx context.Context, ins io.Reader, outs io.Writer, message string) (bool, error) {
	if message == "" {
	message = "Are you sure you want to proceed?"
	}
	message += " [y/N] "
	_, _ = fmt.Fprint(outs, message)

Wondering if;

* haven't checked, but wondered if there's cases where `No` is _not_ the default (so if it should take a default value), but maybe that's not the case.

* ☝️ that said, the `[y/N]` (capital `N`) indicates that `N` is always considered the default; so even if a caller _would_ want to use a different default, the function appending the `[y/N]` means that there's not really a choice for them.

* we should remove the default message (looks like the only place where we pass an empty string is in a test)

* was even considering if we need the `message` at all, because all it does is print the message (unconditionally), and if that should be left to the caller;

* from that perspective I was looking at the `outs io.Writer` and if we need it, but that one looks more tricky, as we still want to print the newline 😓

I agree that the PromptForConfirmation does quite a bit. I would like to also move the code out that catches the os.Signal to somewhere in the main function so we have a top-level way of handling SIGTERM and SIGINT - like we do when executing plugins. But since this has already been quite a big refactor in this PR, I'd like to do this iteratively.

As for default message and default accepted value (N), I kept it as it originally was. The function is explicit that it only asks for a confirmation (yes / no) and does a fallback to no if something other than yes was submitted. I could clean this up more, but I wouldn't want to deviate from the intent of the PR - which is to standardize the exit codes for commands that prompt the user for confirmation. This also standardizes output messages when cancelling the prompt with N/n or SIGINT/SIGTERM.

[laurazard]

Re:

I would like to also move the code out that catches the os.Signal to somewhere in the main function so we have a top-level way of handling SIGTERM and SIGINT - like we do when executing plugins.

I support this, but quick reminder that as we learned with changing signal handling for plugins, these things aren't easy to get right – and we ended up reverting some of the changes/only setting up our own signal handling for plugins when the CLI isn't attached to a terminal. I think we should have some general signal.NotifyContext (or more complex version thereof) and pipe this down into each individual command where it can be handled as appropriate (for example, if a context gets cancelled during a prompt that should be handled differently than during normal command execution), but we must make sure to pipe things into the correct places and not break plugins/places that expect to receive signals.

[thaJeztah]

LGTM, thanks!