Re: [pve-devel] [PATCH proxmox-backup 2/2] client: add 'snapshot notes show/update' command

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/16/20 10:37 AM, Thomas Lamprecht wrote:
> On 13.11.20 10:38, Dominik Csapak wrote:
>> to show and update snapshot notes from the cli
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/bin/proxmox-backup-client.rs          |   1 +
>>   src/bin/proxmox_backup_client/mod.rs      |   2 +
>>   src/bin/proxmox_backup_client/snapshot.rs | 126 ++++++++++++++++++++++
>>   3 files changed, 129 insertions(+)
>>   create mode 100644 src/bin/proxmox_backup_client/snapshot.rs
>>
>> diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
>> index 54e11f08..1581fbd2 100644
>> --- a/src/bin/proxmox-backup-client.rs
>> +++ b/src/bin/proxmox-backup-client.rs
>> @@ -2068,6 +2068,7 @@ fn main() {
>>           .insert("prune", prune_cmd_def)
>>           .insert("restore", restore_cmd_def)
>>           .insert("snapshots", snapshots_cmd_def)
>> +        .insert("snapshot", snapshot_mgtm_cli())
> 
> makes the CLI interface slightly weirder, maybe moving `snapshots`
> into `snapshot list` could help (with hidden alias for backward compat)?

mhmm.. yeah, but we do not have an alias for now AFAIK
i just did not want to add two new top-level commands

> 
> 
>>           .insert("files", files_cmd_def)
>>           .insert("status", status_cmd_def)
>>           .insert("key", key::cli())
>> diff --git a/src/bin/proxmox_backup_client/mod.rs b/src/bin/proxmox_backup_client/mod.rs
>> index 0c4bffb9..a14b0dc1 100644
>> --- a/src/bin/proxmox_backup_client/mod.rs
>> +++ b/src/bin/proxmox_backup_client/mod.rs
>> @@ -8,6 +8,8 @@ mod task;
>>   pub use task::*;
>>   mod catalog;
>>   pub use catalog::*;
>> +mod snapshot;
>> +pub use snapshot::*;
>>   
>>   pub mod key;
>>   
>> diff --git a/src/bin/proxmox_backup_client/snapshot.rs b/src/bin/proxmox_backup_client/snapshot.rs
>> new file mode 100644
>> index 00000000..ee794275
>> --- /dev/null
>> +++ b/src/bin/proxmox_backup_client/snapshot.rs
>> @@ -0,0 +1,126 @@
>> +use anyhow::Error;
>> +use serde_json::{json, Value};
>> +
>> +use proxmox::api::{api, cli::*};
>> +use proxmox_backup::tools;
>> +
>> +use crate::{
>> +    complete_backup_snapshot, connect, extract_repository_from_value, BackupDir, REPO_URL_SCHEMA,
>> +};
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            repository: {
>> +                schema: REPO_URL_SCHEMA,
>> +                optional: true,
>> +            },
>> +            snapshot: {
>> +                type: String,
>> +                description: "Snapshot path.",
>> +            },
>> +            "output-format": {
>> +                schema: OUTPUT_FORMAT,
>> +                optional: true,
>> +            },
>> +        }
>> +    }
>> +)]
>> +/// Show notes
>> +async fn show_notes(param: Value) -> Result<Value, Error> {
>> +    let repo = extract_repository_from_value(&param)?;
>> +    let path = tools::required_string_param(&param, "snapshot")?;
>> +
>> +    let snapshot: BackupDir = path.parse()?;
>> +    let client = connect(&repo)?;
>> +
>> +    let path = format!("api2/json/admin/datastore/{}/notes", repo.store());
>> +
>> +    let args = json!({
>> +        "backup-type": snapshot.group().backup_type(),
>> +        "backup-id": snapshot.group().backup_id(),
>> +        "backup-time": snapshot.backup_time(),
>> +    });
>> +
>> +    let output_format = get_output_format(&param);
> 
> slightly weird to have this here, in between the args value used below, as it's
> only used after that.

ok

> 
>> +
>> +    let mut result = client.get(&path, Some(args)).await?;
>> +
>> +    let comment = result["data"].take();
>> +
>> +    if output_format == "text" {
>> +        if let Some(comment) = comment.as_str() {
>> +            println!("{}", comment);
>> +        }
>> +    } else {
>> +        format_and_print_result(
>> +            &json!({
>> +                "comment": comment,
> 
> We could just return the notes directly, without wrapping into an object?

no, because the proxmox-backup-client calls in pve-storage always use 
--output-format json and tries to decode that (and fails in case of a 
single string without options)

the default call just prints it anyway and having "proper" json makes it 
easier to consume for others
(it depends on the parser if a standalone string is valid json)

> 
>> +            }),
>> +            &output_format,
>> +        );
>> +    }
>> +
>> +    Ok(Value::Null)
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            repository: {
>> +                schema: REPO_URL_SCHEMA,
>> +                optional: true,
>> +            },
>> +            snapshot: {
>> +                type: String,
>> +                description: "Snapshot path.",
>> +            },
>> +            notes: {
>> +                type: String,
>> +                description: "The Notes.",
>> +            },
>> +        }
>> +    }
>> +)]
>> +/// Update Notes
>> +async fn update_notes(param: Value) -> Result<Value, Error> {
>> +    let repo = extract_repository_from_value(&param)?;
>> +    let path = tools::required_string_param(&param, "snapshot")?;
>> +    let notes = tools::required_string_param(&param, "notes")?;
>> +
>> +    let snapshot: BackupDir = path.parse()?;
>> +    let mut client = connect(&repo)?;
>> +
>> +    let path = format!("api2/json/admin/datastore/{}/notes", repo.store());
>> +
>> +    let args = json!({
>> +        "backup-type": snapshot.group().backup_type(),
>> +        "backup-id": snapshot.group().backup_id(),
>> +        "backup-time": snapshot.backup_time(),
>> +        "notes": notes,
>> +    });
>> +
>> +    client.put(&path, Some(args)).await?;
>> +
>> +    Ok(Value::Null)
>> +}
>> +
>> +fn notes_cli() -> CliCommandMap {
>> +    CliCommandMap::new()
>> +        .insert(
>> +            "show",
>> +            CliCommand::new(&API_METHOD_SHOW_NOTES)
>> +                .arg_param(&["snapshot"])
>> +                .completion_cb("snapshot", complete_backup_snapshot),
>> +        )
>> +        .insert(
>> +            "update",
>> +            CliCommand::new(&API_METHOD_UPDATE_NOTES)
>> +                .arg_param(&["snapshot", "notes"])
>> +                .completion_cb("snapshot", complete_backup_snapshot),
>> +        )
>> +}
>> +
>> +pub fn snapshot_mgtm_cli() -> CliCommandMap {
>> +    CliCommandMap::new().insert("notes", notes_cli())
>> +}
>>
> 
>