[pve-devel] [PATCH manager 0/4] followups for vzdump scheduling2

[pve-devel] [PATCH manager 0/4] followups for vzdump scheduling2

[Dominik Csapak]

* fixes the saving/loading of the 'prune-backups' field (oops)
* adds correct validation for id
* adds a comment field for jobs

Dominik Csapak (4):
  Jobs/VZDump: encode/decode 'prune-backups' where needed
  ui: dc/BackupEdit: use correct validation
  api: backup/jobs: add comment field to jobs
  ui: dc/Backup: add comment field and column

 PVE/API2/Backup.pm                 | 14 ++++++++++-
 PVE/Jobs/Plugin.pm                 | 37 ++++++++++++++++++++++++++++++
 PVE/Jobs/VZDump.pm                 | 28 ++++++++++++++++++++++
 www/manager6/dc/Backup.js          | 19 +++++++++++++--
 www/manager6/dc/BackupJobDetail.js |  5 ++++
 5 files changed, 100 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH manager 1/4] Jobs/VZDump: encode/decode 'prune-backups' where needed

[Dominik Csapak]

we want to write it out to the config as propertyString,
but want it as object in the api (for compatiblity)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Jobs/Plugin.pm | 15 +++++++++++++++
 PVE/Jobs/VZDump.pm | 27 +++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
index 69c31cf2..9cf7f98d 100644
--- a/PVE/Jobs/Plugin.pm
+++ b/PVE/Jobs/Plugin.pm
@@ -52,6 +52,21 @@ sub parse_config {
    return $cfg;
 }
 
+# call the plugin specific decode/encode code
+sub decode_value {
+    my ($class, $type, $key, $value) = @_;
+
+    my $plugin = __PACKAGE__->lookup($type);
+    return $plugin->decode_value($type, $key, $value);
+}
+
+sub encode_value {
+    my ($class, $type, $key, $value) = @_;
+
+    my $plugin = __PACKAGE__->lookup($type);
+    return $plugin->encode_value($type, $key, $value);
+}
+
 sub run {
     my ($class, $cfg) = @_;
     # implement in subclass
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index 043b7ace..87733e74 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -7,6 +7,7 @@ use PVE::INotify;
 use PVE::VZDump::Common;
 use PVE::API2::VZDump;
 use PVE::Cluster;
+use PVE::JSONSchema;
 
 use base qw(PVE::Jobs::Plugin);
 
@@ -36,6 +37,32 @@ sub options {
     return $options;
 }
 
+sub decode_value {
+    my ($class, $type, $key, $value) = @_;
+
+    if ($key eq 'prune-backups' && !ref($value)) {
+	$value = PVE::JSONSchema::parse_property_string(
+	    'prune-backups',
+	    $value,
+	);
+    }
+
+    return $value;
+}
+
+sub encode_value {
+    my ($class, $type, $key, $value) = @_;
+
+    if ($key eq 'prune-backups' && ref($value) eq 'HASH') {
+	$value = PVE::JSONSchema::print_property_string(
+	    $value,
+	    'prune-backups',
+	);
+    }
+
+    return $value;
+}
+
 sub run {
     my ($class, $conf) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH manager 2/4] ui: dc/BackupEdit: use correct validation

[Dominik Csapak]

we use a 'pve-configid', so use the 'ConfigId' vtype

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/Backup.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index dede8e6e..24fa94a6 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -181,8 +181,8 @@ Ext.define('PVE.dc.BackupEdit', {
 		name: 'id',
 		fieldLabel: gettext('ID'),
 		renderer: Ext.htmlEncode,
+		vtype: 'ConfigId',
 		allowBlank: false,
-		minLength: 4,
 		editable: me.isCreate,
 	    },
 	    nodesel,
-- 
2.30.2

[pve-devel] [PATCH manager 3/4] api: backup/jobs: add comment field to jobs

[Dominik Csapak]

and encode them with PVE::Tools::encode_text in the config

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Backup.pm | 14 +++++++++++++-
 PVE/Jobs/Plugin.pm | 22 ++++++++++++++++++++++
 PVE/Jobs/VZDump.pm |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 3fa2eba3..428f4cb3 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -168,6 +168,12 @@ __PACKAGE__->register_method({
 		description => "Enable or disable the job.",
 		default => '1',
 	    },
+	    comment => {
+		optional => 1,
+		type => 'string',
+		description => "Description for the Job.",
+		maxLength => 512,
+	    },
        }),
     },
     returns => { type => 'null' },
@@ -363,6 +369,12 @@ __PACKAGE__->register_method({
 		description => "Enable or disable the job.",
 		default => '1',
 	    },
+	    comment => {
+		optional => 1,
+		type => 'string',
+		description => "Description for the Job.",
+		maxLength => 512,
+	    },
        }),
     },
     returns => { type => 'null' },
@@ -417,7 +429,7 @@ __PACKAGE__->register_method({
 	    }
 
 	    foreach my $k (@$delete) {
-		if (!PVE::VZDump::option_exists($k)) {
+		if (!PVE::VZDump::option_exists($k) && $k ne 'comment') {
 		    raise_param_exc({ delete => "unknown option '$k'" });
 		}
 
diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
index 9cf7f98d..cc282850 100644
--- a/PVE/Jobs/Plugin.pm
+++ b/PVE/Jobs/Plugin.pm
@@ -30,6 +30,12 @@ my $defaultData = {
 	    type => 'string', format => 'pve-calendar-event',
 	    maxLength => 128,
 	},
+	comment => {
+	    optional => 1,
+	    type => 'string',
+	    description => "Description for the Job.",
+	    maxLength => 512,
+	},
     },
 };
 
@@ -47,6 +53,10 @@ sub parse_config {
 
 	$data->{id} = $id;
 	$data->{enabled}  //= 1;
+
+	if (defined($data->{comment})) {
+	    $data->{comment} = PVE::Tools::decode_text($data->{comment});
+	}
    }
 
    return $cfg;
@@ -67,6 +77,18 @@ sub encode_value {
     return $plugin->encode_value($type, $key, $value);
 }
 
+sub write_config {
+    my ($class, $filename, $cfg) = @_;
+
+    for my $job (values $cfg->{ids}->%*) {
+	if (defined($job->{comment})) {
+	    $job->{comment} = PVE::Tools::encode_text($job->{comment});
+	}
+    }
+
+    $class->SUPER::write_config($filename, $cfg);
+}
+
 sub run {
     my ($class, $cfg) = @_;
     # implement in subclass
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index 87733e74..92b81147 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -25,6 +25,7 @@ sub options {
     my $options = {
 	enabled => { optional => 1 },
 	schedule => {},
+	comment => { optional => 1 },
     };
     foreach my $opt (keys %$props) {
 	if ($props->{$opt}->{optional}) {
-- 
2.30.2

[pve-devel] [PATCH manager 4/4] ui: dc/Backup: add comment field and column

[Dominik Csapak]

and hide the ID column by default

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/dc/Backup.js          | 17 ++++++++++++++++-
 www/manager6/dc/BackupJobDetail.js |  5 +++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 24fa94a6..45ee7022 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -238,6 +238,14 @@ Ext.define('PVE.dc.BackupEdit', {
 	    onlineHelp: 'chapter_vzdump',
 	    column1: column1,
 	    column2: column2,
+	    columnB: [
+		{
+		    xtype: 'proxmoxtextfield',
+		    name: 'comment',
+		    fieldLabel: gettext('Comment'),
+		    deleteEmpty: !me.isCreate,
+		},
+	    ],
 	    onGetValues: function(values) {
 		if (!values.node) {
 		    if (!me.isCreate) {
@@ -717,6 +725,7 @@ Ext.define('PVE.dc.BackupView', {
 		{
 		    header: gettext('ID'),
 		    dataIndex: 'id',
+		    hidden: true,
 		},
 		{
 		    header: gettext('Node'),
@@ -742,8 +751,14 @@ Ext.define('PVE.dc.BackupView', {
 		    dataIndex: 'storage',
 		},
 		{
-		    header: gettext('Selection'),
+		    header: gettext('Comment'),
+		    dataIndex: 'comment',
+		    renderer: Ext.htmlEncode,
 		    flex: 1,
+		},
+		{
+		    header: gettext('Selection'),
+		    flex: 2,
 		    sortable: false,
 		    dataIndex: 'vmid',
 		    renderer: PVE.Utils.render_backup_selection,
diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
index 19b3b1a3..8300a7d2 100644
--- a/www/manager6/dc/BackupJobDetail.js
+++ b/www/manager6/dc/BackupJobDetail.js
@@ -246,6 +246,11 @@ Ext.define('PVE.dc.BackupInfo', {
     ],
 
     columnB: [
+	{
+	    xtype: 'displayfield',
+	    name: 'comment',
+	    fieldLabel: gettext('Comment'),
+	},
 	{
 	    xtype: 'label',
 	    name: 'pruneLabel',
-- 
2.30.2

Re: [pve-devel] [PATCH manager 0/4] followups for vzdump scheduling2

[Thomas Lamprecht]

On 10.11.21 15:02, Dominik Csapak wrote:
> * fixes the saving/loading of the 'prune-backups' field (oops)
> * adds correct validation for id
> * adds a comment field for jobs

still missing any documentation update ;)

I'd move out the schedule format section from the replication chapter to an appendix
(or some other, more central place), extend backups about the new scheduling stuff.

also the ID is now some in-between stuff, one needs to enter it but it won't be
shown in the grid anyway, so either:

1 go the full way like PBS sync/verify/... jobs and just auto-generate, less inputs
  or the user is normally better UX and they can express comments already in the,
  well new comment field.
2 hide it only for the old ones, rather too confusing
3 allow to edit it for existing jobs, would be a bit weird for the API but as we have
  the digest mechanism and can  do (pseudo call) `PUT /.../backup/<oldid>?id=<newid>`
  it could work without to much confusion..

1 would be the least commitment now, 2 is meh and 3 is more commitment now and may
open a precedent for users to nag about allowing this in more places (where it may
be way harder, a nuisance or plain impossible)

What do you think?

[pve-devel] applied-series: [PATCH manager 0/4] followups for vzdump scheduling2

[Thomas Lamprecht]

On 10.11.21 15:02, Dominik Csapak wrote:
> * fixes the saving/loading of the 'prune-backups' field (oops)
> * adds correct validation for id
> * adds a comment field for jobs
> 
> Dominik Csapak (4):
>   Jobs/VZDump: encode/decode 'prune-backups' where needed
>   ui: dc/BackupEdit: use correct validation
>   api: backup/jobs: add comment field to jobs
>   ui: dc/Backup: add comment field and column
> 
>  PVE/API2/Backup.pm                 | 14 ++++++++++-
>  PVE/Jobs/Plugin.pm                 | 37 ++++++++++++++++++++++++++++++
>  PVE/Jobs/VZDump.pm                 | 28 ++++++++++++++++++++++
>  www/manager6/dc/Backup.js          | 19 +++++++++++++--
>  www/manager6/dc/BackupJobDetail.js |  5 ++++
>  5 files changed, 100 insertions(+), 3 deletions(-)
> 

applied series for now, thanks!