[pbs-devel] [PATCH proxmox-backup] close #4763: client: added command to forget backup group

[pbs-devel] [PATCH proxmox-backup] 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.
Fixed paths for local dependencies in Cargo.toml.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 Cargo.toml                         |  4 +-
 proxmox-backup-client/src/group.rs | 68 ++++++++++++++++++++++++++++++
 proxmox-backup-client/src/main.rs  |  4 +-
 3 files changed, 73 insertions(+), 3 deletions(-)
 create mode 100644 proxmox-backup-client/src/group.rs

diff --git a/Cargo.toml b/Cargo.toml
index c7773f0e..e0f12806 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -264,8 +264,8 @@ proxmox-rrd.workspace = true
 #proxmox-sortable-macro = { path = "../proxmox/proxmox-sortable-macro" }
 #proxmox-human-byte = { path = "../proxmox/proxmox-human-byte" }
 
-#proxmox-apt = { path = "../proxmox-apt" }
-#proxmox-openid = { path = "../proxmox-openid-rs" }
+#proxmox-apt = { path = "../proxmox/proxmox-apt" }
+#proxmox-openid = { path = "../proxmox/proxmox-openid" }
 #pathpatterns = {path = "../pathpatterns" }
 
 #pxar = { path = "../pxar" }
diff --git a/proxmox-backup-client/src/group.rs b/proxmox-backup-client/src/group.rs
new file mode 100644
index 00000000..84b73b67
--- /dev/null
+++ b/proxmox-backup-client/src/group.rs
@@ -0,0 +1,68 @@
+use anyhow::{bail, Error};
+use pbs_api_types::{BackupGroup, BackupNamespace};
+use pbs_client::tools::{connect, extract_repository_from_value};
+use proxmox_router::cli::{ask_for_confirmation, CliCommand, CliCommandMap};
+use proxmox_schema::api;
+use serde_json::Value;
+
+use crate::{
+    complete_backup_group, complete_namespace, complete_repository, merge_group_into,
+    REPO_URL_SCHEMA,
+};
+
+pub fn group_mgtm_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 snapshots == 0 {
+        bail!("Backup group doesn't exist.");
+    }
+
+    ask_for_confirmation(format!(
+        "Delete group \"{}\" with {} snapshots?",
+        backup_group, snapshots
+    ))?;
+
+    let path = format!("api2/json/admin/datastore/{}/groups", repo.store());
+    let _ = client.delete(&path, Some(api_params)).await?;
+
+    println!("Successfully deleted group!");
+    Ok(Value::Null)
+}
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 1a13291a..76d45b84 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;
 
@@ -1606,7 +1608,6 @@ async fn prune(
     }
 
     format_and_print_result_full(&mut data, return_type, &output_format, &options);
-
     Ok(Value::Null)
 }
 
@@ -1783,6 +1784,7 @@ fn main() {
         .insert("benchmark", benchmark_cmd_def)
         .insert("change-owner", change_owner_cmd_def)
         .insert("namespace", namespace::cli_map())
+        .insert("group", group_mgtm_cli())
         .alias(&["files"], &["snapshot", "files"])
         .alias(&["forget"], &["snapshot", "forget"])
         .alias(&["upload-log"], &["snapshot", "upload-log"])
-- 
2.39.2

[pbs-devel] [PATCH proxmox] 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/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;
 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);
+    io::stdout().flush()?;
+    let stdin = io::stdin();
+    let mut line = String::new();
+    stdin.read_line(&mut line)?;
+    if line.trim() == "y" {
+        Ok(())
+    } else {
+        bail!("Aborted");
+    }
+}
+
 /// Define a simple CLI command.
 pub struct CliCommand {
     /// The Schema definition.
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] close #4763: client: added command to forget backup group

[Fiona Ebner]

Am 29.08.23 um 13:13 schrieb Gabriel Goller:
> diff --git a/Cargo.toml b/Cargo.toml
> index c7773f0e..e0f12806 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -264,8 +264,8 @@ proxmox-rrd.workspace = true
>  #proxmox-sortable-macro = { path = "../proxmox/proxmox-sortable-macro" }
>  #proxmox-human-byte = { path = "../proxmox/proxmox-human-byte" }
>  
> -#proxmox-apt = { path = "../proxmox-apt" }
> -#proxmox-openid = { path = "../proxmox-openid-rs" }
> +#proxmox-apt = { path = "../proxmox/proxmox-apt" }
> +#proxmox-openid = { path = "../proxmox/proxmox-openid" }
>  #pathpatterns = {path = "../pathpatterns" }
>  

Should be a separate patch. Each patch should do one thing semantically.

>  #pxar = { path = "../pxar" }
> diff --git a/proxmox-backup-client/src/group.rs b/proxmox-backup-client/src/group.rs
> new file mode 100644
> index 00000000..84b73b67
> --- /dev/null
> +++ b/proxmox-backup-client/src/group.rs
> @@ -0,0 +1,68 @@
> +use anyhow::{bail, Error};
> +use pbs_api_types::{BackupGroup, BackupNamespace};
> +use pbs_client::tools::{connect, extract_repository_from_value};
> +use proxmox_router::cli::{ask_for_confirmation, CliCommand, CliCommandMap};
> +use proxmox_schema::api;
> +use serde_json::Value;
> +

Nit: I think we usually group proxmox includes, pbs includes and other
includes separately.

> +use crate::{
> +    complete_backup_group, complete_namespace, complete_repository, merge_group_into,
> +    REPO_URL_SCHEMA,
> +};
> +
> +pub fn group_mgtm_cli() -> CliCommandMap {

typo: mgtm -> mgmt

(...)

> +    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 snapshots == 0 {
> +        bail!("Backup group doesn't exist.");
> +    }

A group being empty doesn't necessarily mean that it doesn't exist.
Removing an empty group could also be considered valid, to get rid of
the owner, right?

> +
> +    ask_for_confirmation(format!(
> +        "Delete group \"{}\" with {} snapshots?",

Nit: using "snapshot(s)" would also read correctly when it's just 1 snapshot

> +        backup_group, snapshots
> +    ))?;

The second patch you sent should be sorted first, because it is a
prerequisite. Please mention that a dependency bump is needed below the
"---" below the commit message.

> +
> +    let path = format!("api2/json/admin/datastore/{}/groups", repo.store());
> +    let _ = client.delete(&path, Some(api_params)).await?;
> +
> +    println!("Successfully deleted group!");
> +    Ok(Value::Null)
> +}
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 1a13291a..76d45b84 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;
>  
> @@ -1606,7 +1608,6 @@ async fn prune(
>      }
>  
>      format_and_print_result_full(&mut data, return_type, &output_format, &options);
> -
>      Ok(Value::Null)
>  }
>  

This hunk does not belong in this patch either.

Re: [pbs-devel] [PATCH proxmox-backup] close #4763: client: added command to forget backup group

[Gabriel Goller]

On 8/30/23 09:37, Fiona Ebner wrote:
> Am 29.08.23 um 13:13 schrieb Gabriel Goller:
>> diff --git a/Cargo.toml b/Cargo.toml
>> index c7773f0e..e0f12806 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -264,8 +264,8 @@ proxmox-rrd.workspace = true
>>   #proxmox-sortable-macro = { path = "../proxmox/proxmox-sortable-macro" }
>>   #proxmox-human-byte = { path = "../proxmox/proxmox-human-byte" }
>>   
>> -#proxmox-apt = { path = "../proxmox-apt" }
>> -#proxmox-openid = { path = "../proxmox-openid-rs" }
>> +#proxmox-apt = { path = "../proxmox/proxmox-apt" }
>> +#proxmox-openid = { path = "../proxmox/proxmox-openid" }
>>   #pathpatterns = {path = "../pathpatterns" }
>>   
> Should be a separate patch. Each patch should do one thing semantically.
>
>>   #pxar = { path = "../pxar" }
>> diff --git a/proxmox-backup-client/src/group.rs b/proxmox-backup-client/src/group.rs
>> new file mode 100644
>> index 00000000..84b73b67
>> --- /dev/null
>> +++ b/proxmox-backup-client/src/group.rs
>> @@ -0,0 +1,68 @@
>> +use anyhow::{bail, Error};
>> +use pbs_api_types::{BackupGroup, BackupNamespace};
>> +use pbs_client::tools::{connect, extract_repository_from_value};
>> +use proxmox_router::cli::{ask_for_confirmation, CliCommand, CliCommandMap};
>> +use proxmox_schema::api;
>> +use serde_json::Value;
>> +
> Nit: I think we usually group proxmox includes, pbs includes and other
> includes separately.
>
>> +use crate::{
>> +    complete_backup_group, complete_namespace, complete_repository, merge_group_into,
>> +    REPO_URL_SCHEMA,
>> +};
>> +
>> +pub fn group_mgtm_cli() -> CliCommandMap {
> typo: mgtm -> mgmt
>
> (...)
>
>> +    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 snapshots == 0 {
>> +        bail!("Backup group doesn't exist.");
>> +    }
> A group being empty doesn't necessarily mean that it doesn't exist.
> Removing an empty group could also be considered valid, to get rid of
> the owner, right?

IIRC the group gets automatically deleted if there are no snapshots left
(at least it vanishes from the UI)...
So a group with no snapshots means the group doesn't exist.

>> +
>> +    ask_for_confirmation(format!(
>> +        "Delete group \"{}\" with {} snapshots?",
> Nit: using "snapshot(s)" would also read correctly when it's just 1 snapshot
>
>> +        backup_group, snapshots
>> +    ))?;
> The second patch you sent should be sorted first, because it is a
> prerequisite. Please mention that a dependency bump is needed below the
> "---" below the commit message.
>
>> +
>> +    let path = format!("api2/json/admin/datastore/{}/groups", repo.store());
>> +    let _ = client.delete(&path, Some(api_params)).await?;
>> +
>> +    println!("Successfully deleted group!");
>> +    Ok(Value::Null)
>> +}
>> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
>> index 1a13291a..76d45b84 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;
>>   
>> @@ -1606,7 +1608,6 @@ async fn prune(
>>       }
>>   
>>       format_and_print_result_full(&mut data, return_type, &output_format, &options);
>> -
>>       Ok(Value::Null)
>>   }
>>   
> This hunk does not belong in this patch either.

Re: [pbs-devel] [PATCH proxmox-backup] close #4763: client: added command to forget backup group

[Gabriel Goller]

On 8/30/23 09:41, Gabriel Goller wrote:

>
> On 8/30/23 09:37, Fiona Ebner wrote:
>> Am 29.08.23 um 13:13 schrieb Gabriel Goller:
>>> diff --git a/Cargo.toml b/Cargo.toml
>>> index c7773f0e..e0f12806 100644
>>> --- a/Cargo.toml
>>> +++ b/Cargo.toml
>>> @@ -264,8 +264,8 @@ proxmox-rrd.workspace = true
>>>   #proxmox-sortable-macro = { path = 
>>> "../proxmox/proxmox-sortable-macro" }
>>>   #proxmox-human-byte = { path = "../proxmox/proxmox-human-byte" }
>>>   -#proxmox-apt = { path = "../proxmox-apt" }
>>> -#proxmox-openid = { path = "../proxmox-openid-rs" }
>>> +#proxmox-apt = { path = "../proxmox/proxmox-apt" }
>>> +#proxmox-openid = { path = "../proxmox/proxmox-openid" }
>>>   #pathpatterns = {path = "../pathpatterns" }
>> Should be a separate patch. Each patch should do one thing semantically.
>>
>>>   #pxar = { path = "../pxar" }
>>> diff --git a/proxmox-backup-client/src/group.rs 
>>> b/proxmox-backup-client/src/group.rs
>>> new file mode 100644
>>> index 00000000..84b73b67
>>> --- /dev/null
>>> +++ b/proxmox-backup-client/src/group.rs
>>> @@ -0,0 +1,68 @@
>>> +use anyhow::{bail, Error};
>>> +use pbs_api_types::{BackupGroup, BackupNamespace};
>>> +use pbs_client::tools::{connect, extract_repository_from_value};
>>> +use proxmox_router::cli::{ask_for_confirmation, CliCommand, 
>>> CliCommandMap};
>>> +use proxmox_schema::api;
>>> +use serde_json::Value;
>>> +
>> Nit: I think we usually group proxmox includes, pbs includes and other
>> includes separately.
>>
>>> +use crate::{
>>> +    complete_backup_group, complete_namespace, complete_repository, 
>>> merge_group_into,
>>> +    REPO_URL_SCHEMA,
>>> +};
>>> +
>>> +pub fn group_mgtm_cli() -> CliCommandMap {
>> typo: mgtm -> mgmt
>>
>> (...)
>>
>>> +    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 snapshots == 0 {
>>> +        bail!("Backup group doesn't exist.");
>>> +    }
>> A group being empty doesn't necessarily mean that it doesn't exist.
>> Removing an empty group could also be considered valid, to get rid of
>> the owner, right?
>
> IIRC the group gets automatically deleted if there are no snapshots left
> (at least it vanishes from the UI)...
> So a group with no snapshots means the group doesn't exist.

Correction: no it doesn't.

I discussed this with Stefan Sterz and we came to the solution to just 
try and delete the
group even if there are no snapshots in it. This will currently fail 
because there is
this bug open: #3336 (https://bugzilla.proxmox.com/show_bug.cgi?id=3336) 
(TLDR: locking
the OWNER file is difficult).

>
>>> +
>>> +    ask_for_confirmation(format!(
>>> +        "Delete group \"{}\" with {} snapshots?",
>> Nit: using "snapshot(s)" would also read correctly when it's just 1 
>> snapshot
>>
>>> +        backup_group, snapshots
>>> +    ))?;
>> The second patch you sent should be sorted first, because it is a
>> prerequisite. Please mention that a dependency bump is needed below the
>> "---" below the commit message.
>>
>>> +
>>> +    let path = format!("api2/json/admin/datastore/{}/groups", 
>>> repo.store());
>>> +    let _ = client.delete(&path, Some(api_params)).await?;
>>> +
>>> +    println!("Successfully deleted group!");
>>> +    Ok(Value::Null)
>>> +}
>>> diff --git a/proxmox-backup-client/src/main.rs 
>>> b/proxmox-backup-client/src/main.rs
>>> index 1a13291a..76d45b84 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;
>>>   @@ -1606,7 +1608,6 @@ async fn prune(
>>>       }
>>>         format_and_print_result_full(&mut data, return_type, 
>>> &output_format, &options);
>>> -
>>>       Ok(Value::Null)
>>>   }
>> This hunk does not belong in this patch either.
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>

[pbs-devel] [PATCH proxmox] 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>
---

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;
 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);
+    io::stdout().flush()?;
+    let stdin = io::stdin();
+    let mut line = String::new();
+    stdin.read_line(&mut line)?;
+    if line.trim() == "y" {
+        Ok(())
+    } else {
+        bail!("Aborted");
+    }
+}
+
 /// Define a simple CLI command.
 pub struct CliCommand {
     /// The Schema definition.
-- 
2.39.2

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

[Thomas Lamprecht]

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.

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

[Gabriel Goller]

On 9/7/23 17:26, Thomas Lamprecht wrote:
> 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)
Discussed with Max, decided to use `io:Error`.
>>   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.
We only have a helper for the tty output (`TtyOutput`), which is used to
write the asterisks for the password input. I don't think this is useful 
in this case.
>> +    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
Yes, this is nice. I will use `YESEXPR` to match the input from the cli. To
show a localized prompt (the [y/n] thing) I'll extract the lowercase 
character
from the `YESEXPR` and `NOEXPR` (Which is a bit hacky, but sadly the
`NOSTR` and `YESSTR` parameter are deprecated :( ).
> 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.

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

[Gabriel Goller]

Submitted a new version of the series.

On 9/7/23 17:26, Thomas Lamprecht wrote:
> 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.