Re: [pve-devel] [PATCH manager] Delete "exclude" when switching a backup job to pool
Am 8/14/19 um 3:23 PM schrieb Stefan Reiter: >> >> But then the WebGUI would need to be adapted to cope with such a case, >> as currently adding excludes to a pool based backup job results in a >> rather strange and wrong visualization, e.g., "All except 1074" here, >> but all is not selected and editing the job shows an inconsistent wrong >> state too. >> > > That's why I made the patch to the UI, since it is the component that breaks. > If the user knows what they are doing, they can still do so. I see your point > though. > But there can be multiple admins, or some time between the manual change, if then someone tries to edit this over GUI it could be really confusing why the state is as it is, so the backend is the correct way. Possible backend states settable through API should not break GUI (if they're just not exposed in GUI, OK, that's something else) and here most of the time the two options are: * adapt GUI to allow coping with all of the possible backend states * adapt backend to avoid/forbid those states which are not wanted >> >> "Pool Based" backup jobs should stay simple, and as it does not really >> works in all cases I'd rather prevent exclusions with that mode. >> People needing to exclude some VMs can just use other modes.. >> > > In that case I'd say we go for Tims backend version. I'll send the change as > a v2 to make it better to apply. Thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] Delete "exclude" when switching a backup job to pool
On 8/14/19 12:03 PM, Thomas Lamprecht wrote: Am 8/6/19 um 2:41 PM schrieb Stefan Reiter: I don't see a reason to blanket-forbid excluding VMs in pool backups, so I felt leaving the API unchanged was the better option in this case. The GUI is the broken part, the API is working fine, albeit for a use-case it wasn't intentionally designed for. But then the WebGUI would need to be adapted to cope with such a case, as currently adding excludes to a pool based backup job results in a rather strange and wrong visualization, e.g., "All except 1074" here, but all is not selected and editing the job shows an inconsistent wrong state too. That's why I made the patch to the UI, since it is the component that breaks. If the user knows what they are doing, they can still do so. I see your point though. That said, I'm fine with either change. I'd rather say we need both, or? That would be unnecessary, since both patches mentioned here would only lead to deleting the "exclude" data from the update API call. Ignoring external API calls, they basically to the same thing. "Pool Based" backup jobs should stay simple, and as it does not really works in all cases I'd rather prevent exclusions with that mode. People needing to exclude some VMs can just use other modes.. In that case I'd say we go for Tims backend version. I'll send the change as a v2 to make it better to apply. PS: please avoid "top posting" if possible :) ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] Delete "exclude" when switching a backup job to pool
Am 8/6/19 um 2:41 PM schrieb Stefan Reiter:
> I don't see a reason to blanket-forbid excluding VMs in pool backups, so I
> felt leaving the API unchanged was the better option in this case. The GUI is
> the broken part, the API is working fine, albeit for a use-case it wasn't
> intentionally designed for.
>
But then the WebGUI would need to be adapted to cope with such a case,
as currently adding excludes to a pool based backup job results in a
rather strange and wrong visualization, e.g., "All except 1074" here,
but all is not selected and editing the job shows an inconsistent wrong
state too.
> That said, I'm fine with either change.
I'd rather say we need both, or?
"Pool Based" backup jobs should stay simple, and as it does not really
works in all cases I'd rather prevent exclusions with that mode.
People needing to exclude some VMs can just use other modes..
PS: please avoid "top posting" if possible :)
>
> On 8/6/19 2:22 PM, Tim Marx wrote:
>> Thanks for spotting this one.
>> IMHO this would be better fixed in the API, unless we really want to allow
>> pool backups which exclude vms:
>> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
>> index bf9a3330..6c2e16c3 100644
>> --- a/PVE/API2/Backup.pm
>> +++ b/PVE/API2/Backup.pm
>> @@ -476,6 +476,7 @@ __PACKAGE__->register_method({
>> } elsif ($job->{pool}) {
>> delete $job->{vmid};
>> delete $job->{all};
>> + delete $job->{exclude};
>> }
>>> Stefan Reiter hat am 6. August 2019 11:17
>>> geschrieben:
>>>
>>> Previously, if you selected a job in "exclude" mode (in DC GUI) with some
>>> VMIDs
>>> selected and then switched that backup job to "pool", the backup job would
>>> retain the "exclude" section and thus not back up all VMs.
>>>
>>> The backend technically supports this, but the GUI would then misrepresent
>>> this,
>>> showing that all VMs will be backed up (when in reality some will be
>>> excluded)
>>> or straight up break and show "exclude" mode again, with the backend still
>>> being
>>> on "pool".
>>>
>>> Signed-off-by: Stefan Reiter
>>> ---
>>> www/manager6/dc/Backup.js | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
>>> index 6810d92f..79e9cace 100644
>>> --- a/www/manager6/dc/Backup.js
>>> +++ b/www/manager6/dc/Backup.js
>>> @@ -251,6 +251,8 @@ Ext.define('PVE.dc.BackupEdit', {
>>> values.exclude = values.vmid;
>>> delete values.vmid;
>>> } else if (selMode === 'pool') {
>>> + delete values.exclude;
>>> + Proxmox.Utils.assemble_field_data(values, { 'delete':
>>> 'exclude' });
>>> delete values.vmid;
>>> }
>>> --
>>> 2.20.1
>>>
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] Delete "exclude" when switching a backup job to pool
I don't see a reason to blanket-forbid excluding VMs in pool backups, so
I felt leaving the API unchanged was the better option in this case. The
GUI is the broken part, the API is working fine, albeit for a use-case
it wasn't intentionally designed for.
That said, I'm fine with either change.
On 8/6/19 2:22 PM, Tim Marx wrote:
Thanks for spotting this one.
IMHO this would be better fixed in the API, unless we really want to allow pool
backups which exclude vms:
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index bf9a3330..6c2e16c3 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -476,6 +476,7 @@ __PACKAGE__->register_method({
} elsif ($job->{pool}) {
delete $job->{vmid};
delete $job->{all};
+ delete $job->{exclude};
}
Stefan Reiter hat am 6. August 2019 11:17 geschrieben:
Previously, if you selected a job in "exclude" mode (in DC GUI) with some VMIDs
selected and then switched that backup job to "pool", the backup job would
retain the "exclude" section and thus not back up all VMs.
The backend technically supports this, but the GUI would then misrepresent this,
showing that all VMs will be backed up (when in reality some will be excluded)
or straight up break and show "exclude" mode again, with the backend still being
on "pool".
Signed-off-by: Stefan Reiter
---
www/manager6/dc/Backup.js | 2 ++
1 file changed, 2 insertions(+)
diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 6810d92f..79e9cace 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -251,6 +251,8 @@ Ext.define('PVE.dc.BackupEdit', {
values.exclude = values.vmid;
delete values.vmid;
} else if (selMode === 'pool') {
+ delete values.exclude;
+ Proxmox.Utils.assemble_field_data(values, { 'delete':
'exclude' });
delete values.vmid;
}
--
2.20.1
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] Delete "exclude" when switching a backup job to pool
Thanks for spotting this one.
IMHO this would be better fixed in the API, unless we really want to allow pool
backups which exclude vms:
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index bf9a3330..6c2e16c3 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -476,6 +476,7 @@ __PACKAGE__->register_method({
} elsif ($job->{pool}) {
delete $job->{vmid};
delete $job->{all};
+ delete $job->{exclude};
}
> Stefan Reiter hat am 6. August 2019 11:17 geschrieben:
>
>
> Previously, if you selected a job in "exclude" mode (in DC GUI) with some
> VMIDs
> selected and then switched that backup job to "pool", the backup job would
> retain the "exclude" section and thus not back up all VMs.
>
> The backend technically supports this, but the GUI would then misrepresent
> this,
> showing that all VMs will be backed up (when in reality some will be excluded)
> or straight up break and show "exclude" mode again, with the backend still
> being
> on "pool".
>
> Signed-off-by: Stefan Reiter
> ---
> www/manager6/dc/Backup.js | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 6810d92f..79e9cace 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -251,6 +251,8 @@ Ext.define('PVE.dc.BackupEdit', {
> values.exclude = values.vmid;
> delete values.vmid;
> } else if (selMode === 'pool') {
> + delete values.exclude;
> + Proxmox.Utils.assemble_field_data(values, { 'delete':
> 'exclude' });
> delete values.vmid;
> }
>
> --
> 2.20.1
>
>
> ___
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
