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

[Fiona Ebner <f.ebner@proxmox.com>]

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.