[pve-devel] [PATCH manager] ui: mpedit: activate backup on MP creation

[Thomas Lamprecht]

On 11/27/19 4:05 PM, Aaron Lauterer wrote:
> 
> 
> On 11/27/19 12:13 PM, Oguz Bektas wrote:
>> hi,
>>
>> tested and works as expected, although one thing i'd change is the
>> variable name 'isBackedUp'. it sounds a bit like there's already a
>> backup present for the mp.
>>
>> maybe something like 'enableBackup' or similar?
> 
> What about 'isBackupIncluded'? 'enableBackup' might also be a bit too ambiguous. At that step we do not enable or activate any backup, just set the flag to include this mount point IF a backup job is created.

maybe "getsBackedUp"

but not sure about this, it changes consistency with VMs, and we have this
since multiple years as is. Do you have people actively running into such
an situation? I mean the "Backup" checkbox is exactly below the required
"path", so IMO not something to hidden.

But I understand where you come from, maybe we could display it more
visually, e.g. by rendering the backup off setting explicitly as

+-------+-------------------------------------------------------------+
| MP... | local:122/vm-122-disk-0.raw,size=4G  (Excluded from Backup) |
+-------+-------------------------------------------------------------+

? not to sure though.

 
>>
>> On Wed, Nov 27, 2019 at 11:49:38AM +0100, Aaron Lauterer wrote:
>>> This patch enables the backup checkbox by default for newly created
>>> LXC mount points, aligning it with the behaviour when adding disks to
>>> VMs. There new disks are automatically part of backups. If it is not
>>> wanted it needs to be actively disabled in the advanced section.
>>>
>>> Hopefully this will help to avoid situations in the future where people
>>> realize too late that the mount point has not been backed up when they
>>> expected it to because they missed the checkbox.
>>>
>>> The reason why `view.isCreate` is not passed directly is because
>>> AFAIU the 'view.isCreate' can have one of three values:
>>> * null - editing an existing mount point
>>> * true - creating a new mp
>>> * array('unusedX') - adding an unused disk again
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>> ---
>>>
>>> I went with the more verbose `if` because I wasn't sure if a
>>> vm.set ('isBackedUp', !!view.isCreate);
>>> would be okay to use.
>>>
>>>   www/manager6/lxc/MPEdit.js | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/www/manager6/lxc/MPEdit.js b/www/manager6/lxc/MPEdit.js
>>> index 780bef0f..954fa44f 100644
>>> --- a/www/manager6/lxc/MPEdit.js
>>> +++ b/www/manager6/lxc/MPEdit.js
>>> @@ -134,6 +134,11 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>>>           vm.set('node', view.nodename);
>>>           vm.set('unpriv', view.unprivileged);
>>>           vm.set('hideStorSelector', view.unused || !view.isCreate);
>>> +
>>> +        // can be array if created from unused disk
>>> +        if (view.isCreate) {
>>> +        vm.set('isBackedUp', true);
>>> +        }
>>>       }
>>>       },
>>>   @@ -243,7 +248,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>>>           fieldLabel: gettext('Backup'),
>>>           bind: {
>>>           hidden: '{isRoot}',
>>> -        disabled: '{isBindOrRoot}'
>>> +        disabled: '{isBindOrRoot}',
>>> +        value: '{isBackedUp}'
>>>           }
>>>       }
>>>       ],
>>> -- 
>>> 2.20.1
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at pve.proxmox.com
>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>