[pve-devel] [PATCH proxmox-backup/pve-storage/manager] show/edit backup comments

[pve-devel] [PATCH proxmox-backup/pve-storage/manager] show/edit backup comments

[Dominik Csapak]

implement editing/showing comments (or notes) from pve side
for directory and pbs storages

i am not completely happy with the notes != comments mismatch
but we are not even consistent within a product (e.g. description vs comment),
so i am open to suggestions...

pve-manager depends on new pve-storage, which depends on new proxmox-backup-client

proxmox-backup:

Dominik Csapak (2):
  client/http_client: add put method
  client: add 'snapshot notes show/update' command

 src/bin/proxmox-backup-client.rs          |   1 +
 src/bin/proxmox_backup_client/mod.rs      |   2 +
 src/bin/proxmox_backup_client/snapshot.rs | 126 ++++++++++++++++++++++
 src/client/http_client.rs                 |   9 ++
 4 files changed, 138 insertions(+)
 create mode 100644 src/bin/proxmox_backup_client/snapshot.rs

pve-storage:

Dominik Csapak (2):
  api2/storage/content: change to volume_size_info and add return
    properties
  Storage/Plugin: add get/update_volume_comment and implement for
    dir/pbs

 PVE/API2/Storage/Content.pm | 92 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm              | 24 +++++++++-
 PVE/Storage/DirPlugin.pm    | 30 ++++++++++++
 PVE/Storage/PBSPlugin.pm    | 20 ++++++++
 PVE/Storage/Plugin.pm       | 12 +++++
 5 files changed, 171 insertions(+), 7 deletions(-)

pve-manager;

Dominik Csapak (1):
  ui: add ability to show and edit comments for backups

 www/manager6/grid/BackupView.js | 43 ++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

-- 
2.20.1

[pve-devel] [PATCH proxmox-backup 1/2] client/http_client: add put method

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/client/http_client.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index e3f9f323..a9b9c06c 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -534,6 +534,15 @@ impl HttpClient {
         self.request(req).await
     }
 
+    pub async fn put(
+        &mut self,
+        path: &str,
+        data: Option<Value>,
+    ) -> Result<Value, Error> {
+        let req = Self::request_builder(&self.server, self.port, "PUT", path, data)?;
+        self.request(req).await
+    }
+
     pub async fn download(
         &mut self,
         path: &str,
-- 
2.20.1

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

[Dominik Csapak]

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())
         .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);
+
+    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,
+            }),
+            &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())
+}
-- 
2.20.1

[pve-devel] [PATCH storage 1/2] api2/storage/content: change to volume_size_info and add return properties

[Dominik Csapak]

'file_size_info' only works for directory based storages, while
'volume_size_info' should work for all

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Storage/Content.pm | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 8d2ff32..8eeb2eb 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -263,7 +263,30 @@ __PACKAGE__->register_method ({
 	    },
 	},
     },
-    returns => { type => 'object' },
+    returns => {
+	type => 'object',
+	properties => {
+	    path => {
+		description => "The Path",
+		type => 'string',
+	    },
+	    size => {
+		description => "Volume size in bytes.",
+		type => 'integer',
+		renderer => 'bytes',
+	    },
+	    used => {
+		description => "Used space. Please note that most storage plugins " .
+		"do not report anything useful here.",
+		type => 'integer',
+		renderer => 'bytes',
+	    },
+	    format => {
+		description => "Format identifier ('raw', 'qcow2', 'subvol', 'iso', 'tgz' ...)",
+		type => 'string',
+	    },
+	},
+    },
     code => sub {
 	my ($param) = @_;
 
@@ -277,8 +300,8 @@ __PACKAGE__->register_method ({
 	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
 
 	my $path = PVE::Storage::path($cfg, $volid);
-	my ($size, $format, $used, $parent) =  PVE::Storage::file_size_info($path);
-	die "file_size_info on '$volid' failed\n" if !($format && $size);
+	my ($size, $format, $used, $parent) =  PVE::Storage::volume_size_info($cfg, $volid);
+	die "volume_size_info on '$volid' failed\n" if !($format && $size);
 
 	# fixme: return more attributes?
 	return {
-- 
2.20.1

[pve-devel] [PATCH storage 2/2] Storage/Plugin: add get/update_volume_comment and implement for dir/pbs

[Dominik Csapak]

and add the appropriate api call to set and get the comment
we need to bump APIVER for this and can bump APIAGE, since
we only use it at this new call that can work with the default
implementation

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Storage/Content.pm | 63 +++++++++++++++++++++++++++++++++++--
 PVE/Storage.pm              | 24 ++++++++++++--
 PVE/Storage/DirPlugin.pm    | 30 ++++++++++++++++++
 PVE/Storage/PBSPlugin.pm    | 20 ++++++++++++
 PVE/Storage/Plugin.pm       | 12 +++++++
 5 files changed, 145 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 8eeb2eb..acaf8f3 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -285,6 +285,11 @@ __PACKAGE__->register_method ({
 		description => "Format identifier ('raw', 'qcow2', 'subvol', 'iso', 'tgz' ...)",
 		type => 'string',
 	    },
+	    comment => {
+		description => "The optional comment",
+		optional => 1,
+		type => 'string',
+	    }
 	},
     },
     code => sub {
@@ -303,13 +308,67 @@ __PACKAGE__->register_method ({
 	my ($size, $format, $used, $parent) =  PVE::Storage::volume_size_info($cfg, $volid);
 	die "volume_size_info on '$volid' failed\n" if !($format && $size);
 
-	# fixme: return more attributes?
-	return {
+	my $entry = {
 	    path => $path,
 	    size => $size,
             used => $used,
 	    format => $format,
 	};
+
+	# not all storages/types support comments, so ignore errors here
+	eval {
+	    my $comment = PVE::Storage::get_volume_comment($cfg, $volid);
+	    $entry->{comment} = $comment if defined($comment);
+	};
+
+	return $entry;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'updateattributes',
+    path => '{volume}',
+    method => 'PUT',
+    description => "Update volume attributes",
+    permissions => {
+	description => "You need read access for the volume.",
+	user => 'all',
+    },
+    protected => 1,
+    proxyto => 'node',
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id', { optional => 1 }),
+	    volume => {
+		description => "Volume identifier",
+		type => 'string',
+	    },
+	    comment => {
+		description => "The new comment",
+		type => 'string',
+		optional => 1,
+	    },
+	},
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
+	my ($volid, $storeid) = &$real_volume_id($param->{storage}, $param->{volume});
+
+	my $cfg = PVE::Storage::config();
+
+	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
+
+	if (my $comment = $param->{comment}) {
+	    PVE::Storage::update_volume_comment($cfg, $volid, $comment);
+	}
+
+	return undef;
     }});
 
 __PACKAGE__->register_method ({
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 932f0f1..4d1cc5d 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::DRBDPlugin;
 use PVE::Storage::PBSPlugin;
 
 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 7;
+use constant APIVER => 8;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 6;
+use constant APIAGE => 7;
 
 # load standard plugins
 PVE::Storage::DirPlugin->register();
@@ -193,6 +193,26 @@ sub file_size_info {
     return PVE::Storage::Plugin::file_size_info($filename, $timeout);
 }
 
+sub get_volume_comment {
+    my ($cfg, $volid, $timeout) = @_;
+
+    my ($storeid, $volname) = parse_volume_id($volid);
+    my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+    return $plugin->get_volume_comment($scfg, $storeid, $volname, $timeout);
+}
+
+sub update_volume_comment {
+    my ($cfg, $volid, $notes, $timeout) = @_;
+
+    my ($storeid, $volname) = parse_volume_id($volid);
+    my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+    $plugin->update_volume_comment($scfg, $storeid, $volname, $notes, $timeout);
+}
+
 sub volume_size_info {
     my ($cfg, $volid, $timeout) = @_;
 
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 3c81d24..bcc490a 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -87,6 +87,36 @@ sub parse_is_mountpoint {
     return $is_mp; # contains a path
 }
 
+sub get_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+    my $path = $class->filesystem_path($scfg, $volname);
+    $path .= $class->SUPER::COMMENT_EXT;
+
+    my $comment = "";
+
+    if (-f $path) {
+	$comment = PVE::Tools::file_get_contents($path);
+    }
+
+    return $comment;
+}
+
+sub update_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $comment, $timeout) = @_;
+    my $path = $class->filesystem_path($scfg, $volname);
+    my ($vtype, undef, undef, undef, undef, undef, undef) = $class->parse_volname($volname);
+
+    if ($vtype ne 'backup') {
+	die "only backups can have comments\n";
+    }
+
+    $path .= $class->SUPER::COMMENT_EXT;
+
+    PVE::Tools::file_set_contents($path, $comment);
+
+    return undef;
+}
+
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index f3bf016..2bf5cdf 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -590,6 +590,26 @@ sub deactivate_volume {
     return 1;
 }
 
+sub get_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+    my (undef, $name,  undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+
+    my $data = run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "show", $name ]);
+
+    return $data->{comment};
+}
+
+sub update_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $comment, $timeout) = @_;
+
+    my (undef, $name,  undef, undef, undef, undef, $format) = $class->parse_volname($volname);
+
+    run_client_cmd($scfg, $storeid, "snapshot", [ "notes", "update", $name, $comment ], 1);
+
+    return undef;
+}
+
 sub volume_size_info {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index b4f3be8..56398a8 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -813,6 +813,18 @@ sub file_size_info {
     return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
 
+sub get_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+    die "volume comments are not supported for $class";
+}
+
+sub update_volume_comment {
+    my ($class, $scfg, $storeid, $volname, $comment, $timeout) = @_;
+
+    die "volume comments are not supported for $class";
+}
+
 sub volume_size_info {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
     my $path = $class->filesystem_path($scfg, $volname);
-- 
2.20.1

[pve-devel] [PATCH manager 1/1] ui: add ability to show and edit comments for backups

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/grid/BackupView.js | 43 ++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index ec0b60b2..3f06889f 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -227,7 +227,48 @@ Ext.define('PVE.grid.BackupView', {
 	    selModel: sm,
 	    tbar: {
 		overflowHandler: 'scroller',
-		items: [ backup_btn, restore_btn, delete_btn,config_btn, '->', storagesel, '-', vmidfilterCB, storagefilter ],
+		items: [
+		    backup_btn,
+		    restore_btn,
+		    delete_btn,
+		    config_btn,
+		    {
+			xtype: 'proxmoxButton',
+			text: gettext('Edit Comment'),
+			disabled: true,
+			handler: function() {
+			    let volid = sm.getSelection()[0].data.volid;
+			    var storage = storagesel.getValue();
+			    Ext.create('Proxmox.window.Edit', {
+				autoLoad: true,
+				width: 600,
+				height: 400,
+				resizable: true,
+				title: gettext('Comment'),
+				url: `/api2/extjs/nodes/${nodename}/storage/${storage}/content/${volid}`,
+				layout: 'fit',
+				items: [
+				    {
+					xtype: 'textarea',
+					layout: 'fit',
+					name: 'comment',
+					height: '100%',
+				    },
+				],
+				listeners: {
+				    destroy: function() {
+					reload();
+				    },
+				},
+			    }).show();
+			},
+		    },
+		    '->',
+		    storagesel,
+		    '-',
+		    vmidfilterCB,
+		    storagefilter
+		],
 	    },
 	    columns: [
 		{
-- 
2.20.1

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

[Thomas Lamprecht]

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


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

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

> +            }),
> +            &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())
> +}
> 

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

[Dominik Csapak]

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

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

[Thomas Lamprecht]

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.

[pve-devel] applied: [pbs-devel] [PATCH proxmox-backup 1/2] client/http_client: add put method

[Thomas Lamprecht]

On 13.11.20 10:38, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/client/http_client.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
>

applied this one, thanks!