[pbs-devel] [PATCH proxmox{, -backup} v5 0/2] close #4763: client: added command to forget backup group

[pbs-devel] [PATCH proxmox{, -backup} v5 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

Added the command `proxmox-backup-client group forget <group>` so
that we can forget (delete) whole groups with all the containing
snapshots. As this is quite dangerous, we added a prompt, so the
user has to confirm the operation.

v5, thanks @Christian:
 - stricter input matching ($ in regex)
 - avoid printing full datastore path

v4:
 - removed localization

--version 3 has been skipped--

v2:
 - added localization

proxmox:

Gabriel Goller (1):
  router: cli: added `ask_for_confirmation` helper

 proxmox-router/Cargo.toml     |  1 +
 proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)


proxmox-backup:

Gabriel Goller (1):
  close #4763: client: add command to forget backup group

 proxmox-backup-client/src/group.rs | 86 ++++++++++++++++++++++++++++++
 proxmox-backup-client/src/main.rs  |  3 ++
 2 files changed, 89 insertions(+)
 create mode 100644 proxmox-backup-client/src/group.rs


Summary over all repositories:
  4 files changed, 135 insertions(+), 1 deletions(-)

-- 
Generated by git-murpp 0.5.0


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Gabriel Goller]

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>
---
 proxmox-router/Cargo.toml     |  1 +
 proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
index dcd71a4..0b9d361 100644
--- a/proxmox-router/Cargo.toml
+++ b/proxmox-router/Cargo.toml
@@ -19,6 +19,7 @@ percent-encoding.workspace = true
 serde_json.workspace = true
 serde.workspace = true
 unicode-width ="0.1.8"
+regex.workspace = true
 
 # cli:
 tokio = { workspace = true, features = [], optional = true }
diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
index 7df94ad..7f87284 100644
--- a/proxmox-router/src/cli/mod.rs
+++ b/proxmox-router/src/cli/mod.rs
@@ -12,7 +12,10 @@
 //! - 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;
 
@@ -61,6 +64,47 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
     .init();
 }
 
+pub enum DefaultAnswer {
+    Yes,
+    No,
+}
+
+/// Prints a prompt to ask for confirmation
+pub fn ask_for_confirmation(query: String, default: DefaultAnswer) -> Result<bool, io::Error> {
+    let yesnoprompt: (char, char) = match default {
+        DefaultAnswer::Yes => ('Y', 'n'),
+        DefaultAnswer::No => ('y', 'N'),
+    };
+    print!("{query} [{}/{}]: ", yesnoprompt.0, yesnoprompt.1);
+
+    io::stdout().flush()?;
+    let stdin = io::stdin();
+    let mut line = String::new();
+    stdin.read_line(&mut line)?;
+
+    use regex::Regex;
+    match default {
+        DefaultAnswer::Yes => {
+            // unwrap is okay, because this regex is correct
+            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
+            if no_regex.is_match(line.trim()) {
+                Ok(false)
+            } else {
+                Ok(true)
+            }
+        }
+        DefaultAnswer::No => {
+            // unwrap is okay, because this regex is coorrect
+            let yes_regex: Regex = Regex::new("^[yY]$").unwrap();
+            if yes_regex.is_match(line.trim()) {
+                Ok(true)
+            } else {
+                Ok(false)
+            }
+        }
+    }
+}
+
 /// Define a simple CLI command.
 pub struct CliCommand {
     /// The Schema definition.
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v5 2/2] close #4763: client: add command to forget backup group

[Gabriel Goller]

Add the command `proxmox-backup-client group forget <group>` so
that we can forget (delete) whole groups with all the containing
snapshots.
To avoid printing full datastore paths (which are in the error messages)
we filter out the most common one (group not found) and rephrase it.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-backup-client/src/group.rs | 86 ++++++++++++++++++++++++++++++
 proxmox-backup-client/src/main.rs  |  3 ++
 2 files changed, 89 insertions(+)
 create mode 100644 proxmox-backup-client/src/group.rs

diff --git a/proxmox-backup-client/src/group.rs b/proxmox-backup-client/src/group.rs
new file mode 100644
index 00000000..790383bb
--- /dev/null
+++ b/proxmox-backup-client/src/group.rs
@@ -0,0 +1,86 @@
+use anyhow::{bail, Error};
+use serde_json::Value;
+
+use proxmox_router::cli::{ask_for_confirmation, CliCommand, CliCommandMap, DefaultAnswer};
+use proxmox_schema::api;
+
+use crate::{
+    complete_backup_group, complete_namespace, complete_repository, merge_group_into,
+    REPO_URL_SCHEMA,
+};
+use pbs_api_types::{BackupGroup, BackupNamespace};
+use pbs_client::tools::{connect, extract_repository_from_value};
+
+pub fn group_mgmt_cli() -> CliCommandMap {
+    CliCommandMap::new().insert(
+        "forget",
+        CliCommand::new(&API_METHOD_FORGET_GROUP)
+            .arg_param(&["group"])
+            .completion_cb("ns", complete_namespace)
+            .completion_cb("repository", complete_repository)
+            .completion_cb("group", complete_backup_group),
+    )
+}
+
+#[api(
+    input: {
+        properties: {
+            group: {
+                type: String,
+                description: "Backup group",
+            },
+            repository: {
+                schema: REPO_URL_SCHEMA,
+                optional: true,
+            },
+            ns: {
+                type: BackupNamespace,
+                optional: true,
+            },
+        }
+    }
+)]
+/// Forget (remove) backup snapshots.
+async fn forget_group(group: String, param: Value) -> Result<Value, Error> {
+    let backup_group: BackupGroup = group.parse()?;
+    let repo = extract_repository_from_value(&param)?;
+    let client = connect(&repo)?;
+
+    let mut api_params = param;
+    merge_group_into(api_params.as_object_mut().unwrap(), backup_group.clone());
+
+    let path = format!("api2/json/admin/datastore/{}/snapshots", repo.store());
+    let result = client.get(&path, Some(api_params.clone())).await?;
+    let snapshots = result["data"].as_array().unwrap().len();
+
+    if ask_for_confirmation(
+        format!(
+            "Delete group \"{}\" with {} snapshot(s)?",
+            backup_group, snapshots
+        ),
+        DefaultAnswer::No,
+    )? {
+        let path = format!("api2/json/admin/datastore/{}/groups", repo.store());
+        if let Err(err) = client.delete(&path, Some(api_params)).await {
+            // "ENOENT: No such file or directory" is part of the error returned when the group
+            // has not been found. The full error contains the full datastore path and we would
+            // like to avoid printing that to the console. Checking if it exists before deleting
+            // the group doesn't work because we currently do not differentiate between an empty
+            // and a nonexistent group. This would make it impossible to remove empty groups.
+            if err
+                .root_cause()
+                .to_string()
+                .contains("ENOENT: No such file or directory")
+            {
+                bail!("Unable to find backup group!");
+            } else {
+                bail!(err);
+            }
+        }
+        println!("Successfully deleted group!");
+    } else {
+        println!("Abort.");
+    }
+
+    Ok(Value::Null)
+}
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 9dbd3cd1..63669c59 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -72,6 +72,8 @@ mod catalog;
 pub use catalog::*;
 mod snapshot;
 pub use snapshot::*;
+mod group;
+pub use group::*;
 pub mod key;
 pub mod namespace;
 
@@ -1792,6 +1794,7 @@ fn main() {
         .insert("benchmark", benchmark_cmd_def)
         .insert("change-owner", change_owner_cmd_def)
         .insert("namespace", namespace::cli_map())
+        .insert("group", group_mgmt_cli())
         .alias(&["files"], &["snapshot", "files"])
         .alias(&["forget"], &["snapshot", "forget"])
         .alias(&["upload-log"], &["snapshot", "upload-log"])
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Thomas Lamprecht]

Am 18/04/2024 um 13:49 schrieb Gabriel Goller:
> 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>
> ---
>  proxmox-router/Cargo.toml     |  1 +
>  proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
> index dcd71a4..0b9d361 100644
> --- a/proxmox-router/Cargo.toml
> +++ b/proxmox-router/Cargo.toml
> @@ -19,6 +19,7 @@ percent-encoding.workspace = true
>  serde_json.workspace = true
>  serde.workspace = true
>  unicode-width ="0.1.8"
> +regex.workspace = true
>  
>  # cli:
>  tokio = { workspace = true, features = [], optional = true }
> diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> index 7df94ad..7f87284 100644
> --- a/proxmox-router/src/cli/mod.rs
> +++ b/proxmox-router/src/cli/mod.rs
> @@ -12,7 +12,10 @@
>  //! - 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;
>  
> @@ -61,6 +64,47 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
>      .init();
>  }
>  
> +pub enum DefaultAnswer {

The enumeration is not really tied to being a _default_ answer, you just use
it that way now in this patch, I'd rather use just Answer and then also use that
as return value.

See for a replacement proposal at the end.

> +    Yes,
> +    No,
> +}
> +
> +/// Prints a prompt to ask for confirmation
> +pub fn ask_for_confirmation(query: String, default: DefaultAnswer) -> Result<bool, io::Error> {
> +    let yesnoprompt: (char, char) = match default {
> +        DefaultAnswer::Yes => ('Y', 'n'),
> +        DefaultAnswer::No => ('y', 'N'),
> +    };
> +    print!("{query} [{}/{}]: ", yesnoprompt.0, yesnoprompt.1);
> +
> +    io::stdout().flush()?;
> +    let stdin = io::stdin();
> +    let mut line = String::new();
> +    stdin.read_line(&mut line)?;
> +
> +    use regex::Regex;
> +    match default {
> +        DefaultAnswer::Yes => {
> +            // unwrap is okay, because this regex is correct
> +            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> +            if no_regex.is_match(line.trim()) {
> +                Ok(false)
> +            } else {
> +                Ok(true)

So, a caller passes DefaultAnswer::Yes, the user enter the full word "no", your logic
then makes it a Yes like it would for all other unrecognized input...  I do not like
such dangerous interfaces, so NACK.

The default must only be taken if the user did not make any input, but just pressed
enter.

> +            }
> +        }
> +        DefaultAnswer::No => {
> +            // unwrap is okay, because this regex is coorrect
> +            let yes_regex: Regex = Regex::new("^[yY]$").unwrap();
> +            if yes_regex.is_match(line.trim()) {
> +                Ok(true)
> +            } else {
> +                Ok(false)

same here

> +            }
> +        }

I'd rather prefer something like the following interface, it's mostly boilerplate
and docs, but it should allow some safer usage. Could also do s/Confirmation/Confirm/
but no hard feelings there.

The code from patch 2/2 could look the roughly like:

let destroy_group_answer = Confirmation:query_with_default(
    format!("Delete group \"{backup_group}\" with {snapshots} snapshot(s)?"),
    Confirmation:No,
)?;

if destroy_group_answer.is_yes() {
    // ....
}


----8<----
From 6287a4dd3f092e2413356798a41e3a7423536f98 Mon Sep 17 00:00:00 2001
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Date: Wed, 24 Apr 2024 20:55:43 +0200
Subject: [PATCH] router: cli: add yes/no confirmation enum with prompt query
 helper

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 proxmox-router/src/cli/mod.rs | 115 +++++++++++++++++++++++++++++++++-
 1 file changed, 114 insertions(+), 1 deletion(-)

diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
index 7df94ad9..7e561b33 100644
--- a/proxmox-router/src/cli/mod.rs
+++ b/proxmox-router/src/cli/mod.rs
@@ -12,7 +12,10 @@
 //! - 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;
 
@@ -61,6 +64,116 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
     .init();
 }
 
+#[derive(PartialEq, Debug)]
+/// Use for simple yes or no questions, where booleans can be confusing, especially if there's a
+/// default response to consider. The implementation provides query helper for the CLI.
+pub enum Confirmation {
+    Yes,
+    No,
+}
+
+impl Confirmation {
+    /// get the formatted choice for the query prompt, with self being the highlighted (default)
+    /// one displayed as upper case.
+    pub fn default_choice_str(&self) -> &'static str {
+        match self {
+            Self::Yes => "Y/n",
+            Self::No => "y/N",
+        }
+    }
+
+    /// Returns true if the answer is Yes
+    pub fn is_yes(&self) -> bool {
+        *self == Self::Yes
+    }
+
+    /// Returns true if the answer is No
+    pub fn is_no(&self) -> bool {
+        *self == Self::No
+    }
+
+    /// Parse an input string reference as yes or no confirmation.
+    ///
+    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
+    /// 'n', or 'N'. You must trim the string before calling, if needed, or use one of the query
+    /// helper functions.
+    ///
+    /// ```
+    /// use proxmox_router::cli::Confirmation;
+    ///
+    /// let answer = Confirmation::from_str("y");
+    /// assert!(answer.expect("valid").is_yes());
+    ///
+    /// let answer = Confirmation::from_str("N");
+    /// assert!(answer.expect("valid").is_no());
+    ///
+    /// let answer = Confirmation::from_str("bogus");
+    /// assert!(answer.is_err());
+    /// ```
+    pub fn from_str(input: &str) -> Result<Self, io::Error> {
+        match input.trim() {
+            "y" | "Y" => Ok(Self::Yes),
+            "n" | "N" => Ok(Self::No),
+            _ => Err(io::Error::new(
+                io::ErrorKind::InvalidInput,
+                "invalid input, enter one of y, Y, n, or N",
+            )),
+        }
+    }
+
+    /// Parse a input string reference as yes or no confirmation, allowing a fallback default
+    /// answer if the user enters an empty choice.
+    ///
+    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
+    /// 'n', or 'N'. The empty string maps to the default. You must trim the string before calling,
+    /// if needed, or use one of the query helper functions.
+    ///
+    /// ```
+    /// use proxmox_router::cli::Confirmation;
+    ///
+    /// let answer = Confirmation::from_str_with_default("", Confirmation::No);
+    /// assert!(answer.expect("valid").is_no());
+    ///
+    /// let answer = Confirmation::from_str_with_default("n", Confirmation::Yes);
+    /// assert!(answer.expect("valid").is_no());
+    ///
+    /// let answer = Confirmation::from_str_with_default("yes", Confirmation::Yes);
+    /// assert!(answer.is_err()); // full-word answer not allowed for now.
+    /// ```
+    pub fn from_str_with_default(input: &str, default: Self) -> Result<Self, io::Error> {
+        match input.trim() {
+            "y" | "Y" => Ok(Self::Yes),
+            "n" | "N" => Ok(Self::No),
+            "" => Ok(default),
+            _ => Err(io::Error::from(io::ErrorKind::InvalidInput)),
+        }
+    }
+
+    /// Print a query prompt with available yes no choices and returns the String the user enters.
+    fn read_line(query: &str, choices: &str) -> Result<String, io::Error> {
+        print!("{query} [{choices}]: ");
+
+        io::stdout().flush()?;
+        let stdin = io::stdin();
+        let mut line = String::new();
+        stdin.read_line(&mut line)?;
+        Ok(line)
+    }
+
+    /// Print a query prompt and parse the white-space trimmed answer using from_str.
+    pub fn query(query: &str) -> Result<Self, io::Error> {
+        let line = Self::read_line(query, "y/n")?;
+        Confirmation::from_str(line.trim())
+    }
+
+    /// Print a query prompt and parse the answer using from_str_with_default, falling back to the
+    /// default_answer if the user provided an empty string.
+    pub fn query_with_default(query: &str, default_answer: Self) -> Result<Self, io::Error> {
+        let line = Self::read_line(query, default_answer.default_choice_str())?;
+        Confirmation::from_str_with_default(line.trim(), default_answer)
+    }
+}
+
 /// Define a simple CLI command.
 pub struct CliCommand {
     /// The Schema definition.
-- 
2.39.2







_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Gabriel Goller]

On Wed Apr 24, 2024 at 9:03 PM CEST, Thomas Lamprecht wrote:
> Am 18/04/2024 um 13:49 schrieb Gabriel Goller:
> > 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>
> > ---
> >  proxmox-router/Cargo.toml     |  1 +
> >  proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 46 insertions(+), 1 deletion(-)
> > 
> > diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
> > index dcd71a4..0b9d361 100644
> > --- a/proxmox-router/Cargo.toml
> > +++ b/proxmox-router/Cargo.toml
> > @@ -19,6 +19,7 @@ percent-encoding.workspace = true
> >  serde_json.workspace = true
> >  serde.workspace = true
> >  unicode-width ="0.1.8"
> > +regex.workspace = true
> >  
> >  # cli:
> >  tokio = { workspace = true, features = [], optional = true }
> > diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> > index 7df94ad..7f87284 100644
> > --- a/proxmox-router/src/cli/mod.rs
> > +++ b/proxmox-router/src/cli/mod.rs
> > @@ -12,7 +12,10 @@
> >  //! - 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;
> >  
> > @@ -61,6 +64,47 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
> >      .init();
> >  }
> >  
> > +pub enum DefaultAnswer {
>
> The enumeration is not really tied to being a _default_ answer, you just use
> it that way now in this patch, I'd rather use just Answer and then also use that
> as return value.

I agree.

>
> See for a replacement proposal at the end.
>
> > +    Yes,
> > +    No,
> > +}
> > +
> > +/// Prints a prompt to ask for confirmation
> > +pub fn ask_for_confirmation(query: String, default: DefaultAnswer) -> Result<bool, io::Error> {
> > +    let yesnoprompt: (char, char) = match default {
> > +        DefaultAnswer::Yes => ('Y', 'n'),
> > +        DefaultAnswer::No => ('y', 'N'),
> > +    };
> > +    print!("{query} [{}/{}]: ", yesnoprompt.0, yesnoprompt.1);
> > +
> > +    io::stdout().flush()?;
> > +    let stdin = io::stdin();
> > +    let mut line = String::new();
> > +    stdin.read_line(&mut line)?;
> > +
> > +    use regex::Regex;
> > +    match default {
> > +        DefaultAnswer::Yes => {
> > +            // unwrap is okay, because this regex is correct
> > +            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> > +            if no_regex.is_match(line.trim()) {
> > +                Ok(false)
> > +            } else {
> > +                Ok(true)
>
> So, a caller passes DefaultAnswer::Yes, the user enter the full word "no", your logic
> then makes it a Yes like it would for all other unrecognized input...  I do not like
> such dangerous interfaces, so NACK.

I'd have to disagree, I don't think this is a dangerous interface...
If, e.g., we use DefaultAnswer::No, there is no difference between 'no',
'bogus', 'N', or '<enter>' IMO. So why should one return an error and
the other one false?

This was just my thought when implementing this, although ultimately,
it doesn't matter because we only check for success and ignore errors 
and false returns.
FWIW I think your implementation is way prettier, especially the Answer
enum being input and output :)

Should I apply the diff and submit a new version or can this be applied
immediately?

>
> The default must only be taken if the user did not make any input, but just pressed
> enter.
>
> > +            }
> > +        }
> > +        DefaultAnswer::No => {
> > +            // unwrap is okay, because this regex is coorrect
> > +            let yes_regex: Regex = Regex::new("^[yY]$").unwrap();
> > +            if yes_regex.is_match(line.trim()) {
> > +                Ok(true)
> > +            } else {
> > +                Ok(false)
>
> same here
>
> > +            }
> > +        }
>
> I'd rather prefer something like the following interface, it's mostly boilerplate
> and docs, but it should allow some safer usage. Could also do s/Confirmation/Confirm/
> but no hard feelings there.
>
> The code from patch 2/2 could look the roughly like:
>
> let destroy_group_answer = Confirmation:query_with_default(
>     format!("Delete group \"{backup_group}\" with {snapshots} snapshot(s)?"),
>     Confirmation:No,
> )?;
>
> if destroy_group_answer.is_yes() {
>     // ....
> }
>
>
> ----8<----
> From 6287a4dd3f092e2413356798a41e3a7423536f98 Mon Sep 17 00:00:00 2001
> From: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Date: Wed, 24 Apr 2024 20:55:43 +0200
> Subject: [PATCH] router: cli: add yes/no confirmation enum with prompt query
>  helper
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>  proxmox-router/src/cli/mod.rs | 115 +++++++++++++++++++++++++++++++++-
>  1 file changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
> index 7df94ad9..7e561b33 100644
> --- a/proxmox-router/src/cli/mod.rs
> +++ b/proxmox-router/src/cli/mod.rs
> @@ -12,7 +12,10 @@
>  //! - 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;
>  
> @@ -61,6 +64,116 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
>      .init();
>  }
>  
> +#[derive(PartialEq, Debug)]
> +/// Use for simple yes or no questions, where booleans can be confusing, especially if there's a
> +/// default response to consider. The implementation provides query helper for the CLI.
> +pub enum Confirmation {
> +    Yes,
> +    No,
> +}
> +
> +impl Confirmation {
> +    /// get the formatted choice for the query prompt, with self being the highlighted (default)

Nit: 'get' -> 'Get'

> +    /// one displayed as upper case.
> +    pub fn default_choice_str(&self) -> &'static str {
> +        match self {
> +            Self::Yes => "Y/n",
> +            Self::No => "y/N",
> +        }
> +    }
> +
> +    /// Returns true if the answer is Yes
> +    pub fn is_yes(&self) -> bool {
> +        *self == Self::Yes
> +    }
> +
> +    /// Returns true if the answer is No
> +    pub fn is_no(&self) -> bool {
> +        *self == Self::No
> +    }
> +
> +    /// Parse an input string reference as yes or no confirmation.
> +    ///
> +    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
> +    /// 'n', or 'N'. You must trim the string before calling, if needed, or use one of the query
> +    /// helper functions.
> +    ///
> +    /// ```
> +    /// use proxmox_router::cli::Confirmation;
> +    ///
> +    /// let answer = Confirmation::from_str("y");
> +    /// assert!(answer.expect("valid").is_yes());
> +    ///
> +    /// let answer = Confirmation::from_str("N");
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str("bogus");
> +    /// assert!(answer.is_err());
> +    /// ```
> +    pub fn from_str(input: &str) -> Result<Self, io::Error> {
> +        match input.trim() {
> +            "y" | "Y" => Ok(Self::Yes),
> +            "n" | "N" => Ok(Self::No),
> +            _ => Err(io::Error::new(
> +                io::ErrorKind::InvalidInput,
> +                "invalid input, enter one of y, Y, n, or N",
> +            )),
> +        }
> +    }
> +
> +    /// Parse a input string reference as yes or no confirmation, allowing a fallback default
> +    /// answer if the user enters an empty choice.
> +    ///
> +    /// The input string is checked verbatim if it is exactly one of the single chars 'y', 'Y',
> +    /// 'n', or 'N'. The empty string maps to the default. You must trim the string before calling,
> +    /// if needed, or use one of the query helper functions.
> +    ///
> +    /// ```
> +    /// use proxmox_router::cli::Confirmation;
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("", Confirmation::No);
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("n", Confirmation::Yes);
> +    /// assert!(answer.expect("valid").is_no());
> +    ///
> +    /// let answer = Confirmation::from_str_with_default("yes", Confirmation::Yes);
> +    /// assert!(answer.is_err()); // full-word answer not allowed for now.
> +    /// ```
> +    pub fn from_str_with_default(input: &str, default: Self) -> Result<Self, io::Error> {
> +        match input.trim() {
> +            "y" | "Y" => Ok(Self::Yes),
> +            "n" | "N" => Ok(Self::No),
> +            "" => Ok(default),
> +            _ => Err(io::Error::from(io::ErrorKind::InvalidInput)),
> +        }
> +    }
> +
> +    /// Print a query prompt with available yes no choices and returns the String the user enters.
> +    fn read_line(query: &str, choices: &str) -> Result<String, io::Error> {
> +        print!("{query} [{choices}]: ");
> +
> +        io::stdout().flush()?;
> +        let stdin = io::stdin();
> +        let mut line = String::new();
> +        stdin.read_line(&mut line)?;
> +        Ok(line)
> +    }
> +
> +    /// Print a query prompt and parse the white-space trimmed answer using from_str.
> +    pub fn query(query: &str) -> Result<Self, io::Error> {
> +        let line = Self::read_line(query, "y/n")?;
> +        Confirmation::from_str(line.trim())
> +    }
> +
> +    /// Print a query prompt and parse the answer using from_str_with_default, falling back to the
> +    /// default_answer if the user provided an empty string.
> +    pub fn query_with_default(query: &str, default_answer: Self) -> Result<Self, io::Error> {
> +        let line = Self::read_line(query, default_answer.default_choice_str())?;
> +        Confirmation::from_str_with_default(line.trim(), default_answer)
> +    }
> +}
> +
>  /// Define a simple CLI command.
>  pub struct CliCommand {
>      /// The Schema definition.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Thomas Lamprecht]

Am 25/04/2024 um 10:52 schrieb Gabriel Goller:
> On Wed Apr 24, 2024 at 9:03 PM CEST, Thomas Lamprecht wrote:
>> Am 18/04/2024 um 13:49 schrieb Gabriel Goller:
>>> +    use regex::Regex;
>>> +    match default {
>>> +        DefaultAnswer::Yes => {
>>> +            // unwrap is okay, because this regex is correct
>>> +            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
>>> +            if no_regex.is_match(line.trim()) {
>>> +                Ok(false)
>>> +            } else {
>>> +                Ok(true)
>>
>> So, a caller passes DefaultAnswer::Yes, the user enter the full word "no", your logic
>> then makes it a Yes like it would for all other unrecognized input...  I do not like
>> such dangerous interfaces, so NACK.
> 
> I'd have to disagree, I don't think this is a dangerous interface...
> If, e.g., we use DefaultAnswer::No, there is no difference between 'no',
> 'bogus', 'N', or '<enter>' IMO. So why should one return an error and
> the other one false?

I do think there's a big difference though. Because with "" the user entered
the empty choice to get the default that was presented to them, with uppercase
signalling what the default is, and for the others the user entered an unknown,
invalid choice that one must not derive any action from.

And while yes, as long as the default is a no-op it would be indeed fine to
silently ignore any bogus input and relay the default answer, but this here
is a general interface that cannot take such assumptions because it just
cannot know possibly implications of that choice, the default choice is
not bound to be the no-op after all.

E.g., if you have an interface that allows for some changes and then could
sync those to remotes (this is roughly how our CDN repo tooling works).
It's not a real destructive action and most often syncing is what you want,
so defaulting to yes is done. But sometimes one must not sync immediately
due to batching changes or redoing them. Now, what's the better UI for
following prompt + entry

The proposal of the original patch:

```
Sync? [Y/n]: no
Ok, syncing...
```


```
Sync? [Y/n]: no
Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.
```

IMO the latter is much more resilient to human errors, e.g. a middle click
pasting in some text that contains a newline, or a simple misunderstanding.
And FWIW also more unlikely, but not unheard of, things like spilling
drinks, or toddlers or cats walking over the keyboard – which for the
"accept any invalid input as default answer" has a quite high chance to
trigger something unexpected.

In short, for confirmation the behavior should be clear and expected,
accepting arbitrary data as valid choice isn't either; erroring out might
be a nuisance for some, but is safe and provides clarity.


> This was just my thought when implementing this, although ultimately,
> it doesn't matter because we only check for success and ignore errors 
> and false returns.
> FWIW I think your implementation is way prettier, especially the Answer
> enum being input and output :)
> 
> Should I apply the diff and submit a new version or can this be applied
> immediately?

My code was mostly an elaborate sketch, I could be improved a bit, i.e.,
what I did not really thought through is using std::io::Error as Error type,
at least for the "invalid input" case it might be maybe debatable,
but using that now would allow users to avoid forcing the use of a more
complex one, like anyhow, and can also be changed later.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Gabriel Goller]

On Thu Apr 25, 2024 at 11:42 AM CEST, Thomas Lamprecht wrote:
> Am 25/04/2024 um 10:52 schrieb Gabriel Goller:
> > On Wed Apr 24, 2024 at 9:03 PM CEST, Thomas Lamprecht wrote:
> >> Am 18/04/2024 um 13:49 schrieb Gabriel Goller:
> >>> +    use regex::Regex;
> >>> +    match default {
> >>> +        DefaultAnswer::Yes => {
> >>> +            // unwrap is okay, because this regex is correct
> >>> +            let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> >>> +            if no_regex.is_match(line.trim()) {
> >>> +                Ok(false)
> >>> +            } else {
> >>> +                Ok(true)
> >>
> >> So, a caller passes DefaultAnswer::Yes, the user enter the full word "no", your logic
> >> then makes it a Yes like it would for all other unrecognized input...  I do not like
> >> such dangerous interfaces, so NACK.
> > 
> > I'd have to disagree, I don't think this is a dangerous interface...
> > If, e.g., we use DefaultAnswer::No, there is no difference between 'no',
> > 'bogus', 'N', or '<enter>' IMO. So why should one return an error and
> > the other one false?
>
> I do think there's a big difference though. Because with "" the user entered
> the empty choice to get the default that was presented to them, with uppercase
> signalling what the default is, and for the others the user entered an unknown,
> invalid choice that one must not derive any action from.
>
> And while yes, as long as the default is a no-op it would be indeed fine to
> silently ignore any bogus input and relay the default answer, but this here
> is a general interface that cannot take such assumptions because it just
> cannot know possibly implications of that choice, the default choice is
> not bound to be the no-op after all.
>
> E.g., if you have an interface that allows for some changes and then could
> sync those to remotes (this is roughly how our CDN repo tooling works).
> It's not a real destructive action and most often syncing is what you want,
> so defaulting to yes is done. But sometimes one must not sync immediately
> due to batching changes or redoing them. Now, what's the better UI for
> following prompt + entry
>
> The proposal of the original patch:
>
> ```
> Sync? [Y/n]: no
> Ok, syncing...
> ```
>
>
> ```
> Sync? [Y/n]: no
> Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.
> ```
>
> IMO the latter is much more resilient to human errors, e.g. a middle click
> pasting in some text that contains a newline, or a simple misunderstanding.
> And FWIW also more unlikely, but not unheard of, things like spilling
> drinks, or toddlers or cats walking over the keyboard – which for the
> "accept any invalid input as default answer" has a quite high chance to
> trigger something unexpected.
>
> In short, for confirmation the behavior should be clear and expected,
> accepting arbitrary data as valid choice isn't either; erroring out might
> be a nuisance for some, but is safe and provides clarity.

Ok, you convinced me :)

> > This was just my thought when implementing this, although ultimately,
> > it doesn't matter because we only check for success and ignore errors 
> > and false returns.
> > FWIW I think your implementation is way prettier, especially the Answer
> > enum being input and output :)
> > 
> > Should I apply the diff and submit a new version or can this be applied
> > immediately?
>
> My code was mostly an elaborate sketch, I could be improved a bit, i.e.,
> what I did not really thought through is using std::io::Error as Error type,
> at least for the "invalid input" case it might be maybe debatable,
> but using that now would allow users to avoid forcing the use of a more
> complex one, like anyhow, and can also be changed later.

The current output on error is: 

    Error: invalid input parameter

anyhow is already used in proxmox-router so I'd say we can just as well
use it. A simple bail! with the message you proposed should do the trick:

    Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox v5 1/2] router: cli: added `ask_for_confirmation` helper

[Thomas Lamprecht]

Am 25/04/2024 um 12:37 schrieb Gabriel Goller:
> anyhow is already used in proxmox-router so I'd say we can just as well
> use it. A simple bail! with the message you proposed should do the trick:
> 
>     Abort – unexpected choice "no"! Use enter for default or use 'y' or 'n'.


I'd drop the "Abort -" here and bail only with the "unexpected choice "{input}"!
Use enter for default or use 'y' or 'n'" here, as what the calling code then
does is their decision, might not necessarily be aborting the current operation,
but re-asking the user again.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} v5 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

submitted v6! 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel