[pve-devel] [PATCH manager] Improve storage selection on restore

[Fabian Ebner]

On 5/5/20 1:40 PM, Thomas Lamprecht wrote:
> On 5/5/20 1:20 PM, Fabian Ebner wrote:
>> Previously, the blank '' would be passed along and lead to a
>> parameter verfication failure.
>>
>> For LXC the default behavior in the backend is to use 'local' as
>> the storage, so disallow blank and auto-select the first storage
>> supporting 'rootdir' instead.
>>
>> For QEMU the default behavior in the backend is to use the
>> original layout from the backup configuration file, which
>> makes sense to use as the default in the GUI as well.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   www/manager6/window/Restore.js | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
>> index 3f9ad3bc..09c8a997 100644
>> --- a/www/manager6/window/Restore.js
>> +++ b/www/manager6/window/Restore.js
>> @@ -24,7 +24,11 @@ Ext.define('PVE.window.Restore', {
>>   	    value: '',
>>   	    fieldLabel: gettext('Storage'),
>>   	    storageContent: (me.vmtype === 'lxc') ? 'rootdir' : 'images',
>> -	    allowBlank: true
>> +	    // when restoring a container without specifying a storage, the backend defaults
>> +	    // to 'local', which is unintuitive and 'rootdir' might not even be allowed on it
>> +	    allowBlank: (me.vmtype === 'lxc') ? false : true,
>> +	    emptyText: (me.vmtype === 'lxc') ? '' : gettext('use old layout'),
> 
> 1. We capitalize gettext strings normally, at least the first word
> 2. "Use old layout" is quite confusing, IMO, maybe "From Backup Configuration"
>     or "Backup Layout"
> 

Yes, "From Backup Configuration" is better.

> I mean in the longer run we want to probably stream line behavior between CT
> and VM so that both try to use the ones from the backup config or even pass
> a volume to storage map (as advanced feature).
> 
> 
>> +	    autoSelect: (me.vmtype === 'lxc') ? true : false,
> 
> in general I like to avoid ternary operations if easily possible, for boolean
> rvalues that's a must, me.vmtype === 'lxc' is already true or false ;-)
> 
> 

I thought it might make sense for readability in this case, because 
having the same expression four times in a row, one can read the first 
"column" as what happens for LXC and the second "column" as what happens 
for QEMU. But I can of course change it in v2.

>>   	});
>>   
>>   	var IDfield;
>> @@ -135,16 +139,15 @@ Ext.define('PVE.window.Restore', {
>>   	var submitBtn = Ext.create('Ext.Button', {
>>   	    text: gettext('Restore'),
>>   	    handler: function(){
>> -		var storage = storagesel.getValue();
>>   		var values = form.getValues();
>>   
>>   		var params = {
>> -		    storage: storage,
>>   		    vmid: me.vmid || values.vmid,
>>   		    force: me.vmid ? 1 : 0
>>   		};
>>   		if (values.unique) { params.unique = 1; }
>>   		if (values.start) { params.start = 1; }
>> +		if (values.storage) { params.storage = values.storage; }
>>   
>>   		if (values.bwlimit !== undefined) {
>>   		    params.bwlimit = values.bwlimit;
>>
>