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: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
>
>