cli icon indicating copy to clipboard operation
cli copied to clipboard

feat: standardize error for prompt

Open Benehiko opened this issue 1 year ago • 1 comments

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)

Benehiko avatar Mar 12 '24 11:03 Benehiko

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%.

: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     

codecov-commenter avatar Mar 18 '24 09:03 codecov-commenter

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

Benehiko avatar Mar 20 '24 16:03 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

:thinking:

Wonder if some flakiness has been introduced in this commit https://github.com/docker/cli/commit/d68cc0e8d0164ab5b3f8cd47b333891604f4c4c9#diff-2b3ea721470acb354ca37a32dc40b7c0ae55bd43397a084adbd922fca5964e20

Benehiko avatar Mar 22 '24 12:03 Benehiko

merge!

Benehiko avatar Mar 26 '24 14:03 Benehiko

@thaJeztah care to take a final look at this?

laurazard avatar Mar 27 '24 12:03 laurazard

Could we merge this one?

Benehiko avatar Apr 02 '24 06:04 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

https://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.

Benehiko avatar Apr 02 '24 12:04 Benehiko

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.

laurazard avatar Apr 02 '24 14:04 laurazard