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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 16.11.20 10:42, Dominik Csapak wrote:
> 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

shouldn't be to hard, it's probably just a flag to hide it from the help
message, but not to hard feelings, rather pointed it out - some additional
opinions would be good here.

> i just did not want to add two new top-level commands

that is appreciated.

>>> +
>>> +    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)
> 

But
"foo"

*is* valid JSON...

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

the standard says so, both chromium's and Firefox JSON.parse('"foo"') work
as expected, so if serde can work with that I'd say a standard conform JSON
parser is a OK requirement in general. Note not saying that we thus must use
it, but that we could just fine if it really makes sense for an endpoint.

Albeit, PBS seems to also return an object:
$ GET /api2/json/admin/datastore/test/notes?backup-type=vm&backup-id=101&backup-time=1598010396

{"data":"test comment 1234"}

so not sure how close we should mirror this, I'm maybe just not that content with "notes" /
"comment" discrepancy between the two.