[pve-devel] [PATCH manager] api: backup: auto-inject job id where expected by the API

[pve-devel] [PATCH manager] api: backup: auto-inject job id where expected by the API

[Fiona Ebner]

for backwards compatibility. Otherwise, e.g. listing backup jobs with
pvesh get /cluster/backup is broken. And suddenly not having the
property anymore would be a breaking API change.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

I didn't see any other places that rely on the $job->{id} to be set.
The other backup-related API calls and Jobs.pm iterate or check the
ids from the section config's $parsed->{ids}. Scheduler doesn't need
either AFAICT.

@Dominik: I guess you need to adapt the (not-yet-applied) realm sync
jobs API too now that the id is not auto-injected.

 PVE/API2/Backup.pm | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index b6f5916d..3a079874 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -130,6 +130,10 @@ __PACKAGE__->register_method({
 		$job->{'next-run'} = $next_run if defined($next_run);
 	    }
 
+	    # FIXME remove in PVE 8.0?
+	    # backwards compat: before moving the job registry to pve-common, id was auto-injected
+	    $job->{id} = $jobid;
+
 	    push @$res, $job;
 	}
 
@@ -273,7 +277,12 @@ __PACKAGE__->register_method({
 
 	my $jobs_data = cfs_read_file('jobs.cfg');
 	my $job = $jobs_data->{ids}->{$param->{id}};
-	return $job if $job && $job->{type} eq 'vzdump';
+	if ($job && $job->{type} eq 'vzdump') {
+	    # FIXME remove in PVE 8.0?
+	    # backwards compat: before moving the job registry to pve-common, id was auto-injected
+	    $job->{id} = $param->{id};
+	    return $job;
+	}
 
 	raise_param_exc({ id => "No such job '$param->{id}'" });
 
-- 
2.30.2

[pve-devel] [PATCH common] job registry: remove id property

[Fiona Ebner]

Since VZDump/JobBase.pm in guest-common doesn't declare that it has an
option 'id', this is unused anyway and the auto-inject logic wouldn't
trigger.

Alternatively, the 'id' option could be added in VZDump/JobBase.pm (as
optional, because existing jobs.cfg don't contain the id as a property
within the section), but it would lead to the id being written to the
section in write_config, which is not nice at all as the id is already
in the header.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Job/Registry.pm | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/src/PVE/Job/Registry.pm b/src/PVE/Job/Registry.pm
index 32e0272..03bed5e 100644
--- a/src/PVE/Job/Registry.pm
+++ b/src/PVE/Job/Registry.pm
@@ -18,14 +18,6 @@ use base qw(PVE::SectionConfig);
 my $defaultData = {
     propertyList => {
 	type => { description => "Section type." },
-	# FIXME: remove below? this is the section ID, schema would only be checked if a plugin
-	# declares this as explicit option, which isn't really required as its available anyway..
-	id => {
-	    description => "The ID of the job.",
-	    type => 'string',
-	    format => 'pve-configid',
-	    maxLength => 64,
-	},
 	enabled => {
 	    description => "Determines if the job is enabled.",
 	    type => 'boolean',
@@ -66,11 +58,6 @@ sub parse_config {
 	my $data = $cfg->{ids}->{$id};
 	my $type = $data->{type};
 
-	# FIXME: below id injection is gross, guard to avoid breaking plugins that don't declare id
-	# as option; *iff* we want this it should be handled by section config directly.
-	if ($defaultData->{options}->{$type} && exists $defaultData->{options}->{$type}->{id}) {
-	    $data->{id} = $id;
-	}
 	$data->{enabled}  //= 1;
 
 	$data->{comment} = PVE::Tools::decode_text($data->{comment}) if defined($data->{comment});
-- 
2.30.2

Re: [pve-devel] [PATCH manager] api: backup: auto-inject job id where expected by the API

[Fiona Ebner]

The commit message should include the why too of course rather than just
having it in the code comments:

Am 15.11.22 um 11:18 schrieb Fiona Ebner:
> for backwards compatibility.

When moving the job base plugin to pve-common, the behavior for parsing
the job config changed slightly, and 'id' was not automatically set as a
job property anymore.

> Otherwise, e.g. listing backup jobs with
> pvesh get /cluster/backup is broken. And suddenly not having the
> property anymore would be a breaking API change.
> 

Sorry for the noise.

[pve-devel] applied: [PATCH manager] api: backup: auto-inject job id where expected by the API

[Thomas Lamprecht]

Am 15/11/2022 um 11:18 schrieb Fiona Ebner:
> for backwards compatibility. Otherwise, e.g. listing backup jobs with
> pvesh get /cluster/backup is broken. And suddenly not having the
> property anymore would be a breaking API change.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> I didn't see any other places that rely on the $job->{id} to be set.
> The other backup-related API calls and Jobs.pm iterate or check the
> ids from the section config's $parsed->{ids}. Scheduler doesn't need
> either AFAICT.
> 
> @Dominik: I guess you need to adapt the (not-yet-applied) realm sync
> jobs API too now that the id is not auto-injected.
> 
>  PVE/API2/Backup.pm | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
>

applied, thanks!