Re: [pbs-devel] [PATCH proxmox] router: cli: added `ask_for_confirmation` helper

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 30/08/2023 14:45, Gabriel Goller wrote:
> Added `ask_for_confirmation` helper that outputs a prompt and
> lets the user confirm or deny it. Implemented to close #4763.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> Note: A dependency bump is needed.
> 
>  proxmox-router/src/cli/mod.rs | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> index 208df4a..2366b47 100644
> --- a/proxmox-router/src/cli/mod.rs
> +++ b/proxmox-router/src/cli/mod.rs
> @@ -12,11 +12,15 @@
>  //! - Ability to create interactive commands (using ``rustyline``)
>  //! - Supports complex/nested commands
>  
> -use std::collections::HashMap;
> +use std::{
> +    collections::HashMap,
> +    io::{self, Write},
> +};
>  
>  use crate::ApiMethod;
>  
>  mod environment;
> +use anyhow::bail;

hmm, anyhow on library code is a slight smell, not sure about our verdict
here, for API stuff it's widely used anyway – @Wolfgang (or @Max, you checked
out error handling too here IIRC) 

>  pub use environment::*;
>  
>  mod shellword;
> @@ -62,6 +66,19 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
>      .init();
>  }
>  
> +pub fn ask_for_confirmation(query: String) -> Result<(), anyhow::Error> {
> +    print!("{} [y/N]: ", query);

here too w.r.t. format strings and params please:

"{query} [y/N]"

> +    io::stdout().flush()?;
> +    let stdin = io::stdin();
> +    let mut line = String::new();
> +    stdin.read_line(&mut line)?;

don't we have already some pty/tty helpers that we use for passwords that
we could re-use here? Not a hard recommendation or the like, just interest.

> +    if line.trim() == "y" {

Hmm, not locale-aware though. Albeit we do not place that much of an
emphasis on being so in our CLI tools.

Would be rather just interesting if there are sane wrappers or
implementations for something like nl_langinfo's [0] YESEXPR or NOEXPR
fom libc in rust.

[0]: https://manpages.debian.org/bookworm/manpages-dev/nl_langinfo.3.en.html

Anyhow, not blocking this, just stuck out – would at least compare
the char case-insensitive though.

> +        Ok(())
> +    } else {
> +        bail!("Aborted");
> +    }
> +}
> +
>  /// Define a simple CLI command.
>  pub struct CliCommand {
>      /// The Schema definition.