Re: [pve-devel] [PATCH v2 manager 2/3] ui: restore: allow override of some settings

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 21.04.22 13:26, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v2.
> 
> Dependency bump for qemu-server needed
> 
>  www/manager6/window/Restore.js | 77 +++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
> index 25babf89..23c244f3 100644
> --- a/www/manager6/window/Restore.js
> +++ b/www/manager6/window/Restore.js
> @@ -21,6 +21,7 @@ Ext.define('PVE.window.Restore', {
>  
>  	    Proxmox.Utils.API2Request({
>  		url: `/nodes/${view.nodename}/vzdump/extractconfig`,
> +		waitMsgTarget: view,
>  		method: 'GET',
>  		params: {
>  		    volume: view.volid,
> @@ -38,6 +39,28 @@ Ext.define('PVE.window.Restore', {
>  				`storage/${view.nodename}/${match[3]}`,
>  			    );
>  			    storagesAvailable = storagesAvailable && currentAvailable;
> +			} else {

hmm, ok you reuse the forEach, so the every of the previous patch review may not work, but I'd 
still do some changes, see below.

> +			    match = line.match(/^([^:]+):\s*(\S+)\s*$/);

can be: 

let [_, key, value] = line.match(/^([^:]+):\s*(\S+)\s*$/) ?? [];

if (!key) {
    return;
}

...

And then this could be move out of this if and handled like:

if (key === #qmdump#map) {
     // ...
} else if (key === 'name' || 'hostname') {
     view.lookupReference('nameField').setEmptyText(value);
) else if (key === 'memory') {
   // ...
} ...

Alternatively a single else and an object to map from key to xField would be also an option, but the
switch is almost never too verbose and often, and that's probably just my opinion, slightly consufing
to parse when skimming over code quickly.

> +			    if (match) {
> +				let [_, key, value] = match;
> +				switch (key) {
> +				    case 'name':
> +				    case 'hostname':
> +					view.lookupReference('nameField').setEmptyText(value);
> +					break;
> +				    case 'memory':
> +					view.lookupReference('memoryField').setEmptyText(value);
> +					break;
> +				    case 'cores':
> +					view.lookupReference('coresField').setEmptyText(value);
> +					break;
> +				    case 'sockets':
> +					view.lookupReference('socketsField').setEmptyText(value);
> +					break;
> +				    default:
> +					break;
> +				}
> +			    }
>  			}
>  		    });
>  
> @@ -207,6 +230,49 @@ Ext.define('PVE.window.Restore', {
>  	    });
>  	}
>  
> +	items.push(
> +	    {
> +		xtype: 'displayfield',
> +		value: `${gettext('Override Settings')}:`,

I'd maybe even use a 'fieldset' widget here for grouping this nicely.

> +	    },
> +	    {
> +		xtype: 'textfield',
> +		fieldLabel: gettext('Name'),
> +		name: 'name',
> +		reference: 'nameField',
> +		allowBlank: true,
> +	    },
> +	    {
> +		xtype: 'pveMemoryField',
> +		fieldLabel: gettext('Memory'),
> +		name: 'memory',
> +		reference: 'memoryField',
> +		value: '',
> +		allowBlank: true,
> +	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		fieldLabel: gettext('Cores'),
> +		name: 'cores',
> +		reference: 'coresField',
> +		minValue: 1,
> +		maxValue: 128,
> +		allowBlank: true,
> +	    },
> +	);
> +
> +	if (me.vmtype === 'qemu') {
> +	    items.push({
> +		xtype: 'proxmoxintegerfield',
> +		fieldLabel: gettext('Sockets'),
> +		name: 'sockets',
> +		reference: 'socketsField',
> +		minValue: 1,
> +		maxValue: 4,
> +		allowBlank: true,
> +	    });
> +	}
> +
>  	me.formPanel = Ext.create('Ext.form.Panel', {
>  	    bodyPadding: 10,
>  	    border: false,
> @@ -254,8 +320,15 @@ Ext.define('PVE.window.Restore', {
>  		if (values['live-restore']) { params['live-restore'] = 1; }
>  		if (values.storage) { params.storage = values.storage; }
>  
> -		if (values.bwlimit !== undefined) {
> -		    params.bwlimit = values.bwlimit;
> +		['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(function(opt) {
> +		    if (values[opt] !== undefined && values[opt] !== null && values[opt] !== '') {

should be collapsible via:

if ((values[opt] ?? '') !== '') 

at which point we could move it into a filter combinator, like:

['bwlimit', 'cores', 'name', 'memory', 'sockets']
    .filter(opt => (values[opt] ?? '') !== '')
    .forEach(opt => param[opt] = values[opt]);

> +			params[opt] = values[opt];
> +		    }
> +		});
> +
> +		if (params.name && me.vmtype === 'lxc') {
> +		    params.hostname = params.name;
> +		    delete params.name;
>  		}
>  
>  		var url;