feat: standardize error for prompt
Follow up to https://github.com/docker/cli/pull/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 Report
Merging #4939 (7c722c0) into master (b8d5454) will decrease coverage by
0.04%. Report is 8 commits behind head on master. The diff coverage is65.78%.
:exclamation: 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
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 https://github.com/docker/cli/issues/4948
--- 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
:thinking:
Wonder if some flakiness has been introduced in this commit https://github.com/docker/cli/commit/d68cc0e8d0164ab5b3f8cd47b333891604f4c4c9#diff-2b3ea721470acb354ca37a32dc40b7c0ae55bd43397a084adbd922fca5964e20

@thaJeztah care to take a final look at this?
Could we merge this one?
Thanks! Overall looks good; had a question about that magic
UNIQUEMEvariable, 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 muchhttps://github.com/docker/cli/blob/155dc5e4e406f935965260ac45524f950df0d920/cli/command/utils.go#L92-L98
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.
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.