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

[Gabriel Goller <g.goller@proxmox.com>]

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.