Re: [pve-devel] [PATCH manager 2/3] Hardware View: Add GUI for importdisk

[Aaron Lauterer <a.lauterer@proxmox.com>]

Some thing inline.

On 7/7/20 12:04 PM, Dominic Jäger wrote:
> Make importing single disks easier.
> Required to import a whole VM via GUI.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>   www/manager6/qemu/HDEdit.js       | 80 ++++++++++++++++++++++---------
>   www/manager6/qemu/HardwareView.js | 18 +++++++
>   2 files changed, 76 insertions(+), 22 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index e2a5b914..7f6a7b2b 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -76,23 +76,43 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   	    me.drive.format = values.diskformat;
>   	}
>   
> -	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> -	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> -	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> -
> -        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> -        Ext.Array.each(names, function(name) {
> -            var burst_name = name + '_max';
> -	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> -	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> -        });
> -
> -
> -	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	if (me.isImport) {
> +	    // These keys & values are accepted by the API as they are
> +	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
> +	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +	    burst = burst.concat(burst.map(x => `${x}_max`));
> +	    let available = simple.concat(burst);
> +	    let addValues = key => `${key}=${values[key]}`;
> +	    let selectedKeys = x => values[x];
> +	    let options = available.filter(selectedKeys).map(addValues).join();
> +	    // These need modification for the API
> +	    options += values.discard ? ',discard=on' : '';
> +	    options += values.noreplicate ? ',replicate=0' : '';
> +	    params.device_options = options;

One or two newlines in the above block would be nice to make the distinction between the logical blocks more obvious.

> +	} else {
> +	    PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> +	    PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> +	    PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> +
> +	    var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +		Ext.Array.each(names, function(name) {
> +		    var burst_name = name + '_max';
> +		    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> +		    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> +	    });
> +	}
>   
> +	if (me.isImport) {
> +	    params.source = values.inputImage;
> +	    params.device = values.controller + values.deviceid;
> +	    params.storage = values.hdstorage;
> +	    if (values.diskformat) params.format = values.diskformat;
> +	} else {
> +	    params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	}
>   	return params;
>       },
>   
> @@ -199,14 +219,17 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		allowBlank: false
>   	    });
>   	    me.column1.push(me.unusedDisks);
> -	} else if (me.isCreate) {
> -	    me.column1.push({
> +	} else if (me.isCreate || me.isImport) {
> +	    let selector = {
>   		xtype: 'pveDiskStorageSelector',
>   		storageContent: 'images',
>   		name: 'disk',
>   		nodename: me.nodename,
> -		autoSelect: me.insideWizard
> -	    });
> +		hideSize: me.isImport,
> +		autoSelect: me.insideWizard || me.isImport,
> +	    };
> +	    if (me.isImport) selector.storageLabel = 'Target storage';

this should be a `gettext('Target storage')` so it can be translated.

> +	    me.column1.push(selector);
>   	} else {
>   	    me.column1.push({
>   		xtype: 'textfield',
> @@ -231,6 +254,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		name: 'discard'
>   	    }
>   	);
> +	if (me.isImport) {
> +	    me.column2.push({
> +		xtype: 'textfield',
> +		fieldLabel: gettext('Source image'),
> +		name: 'inputImage',
> +		emptyText: '/home/user/disk.qcow2',
> +	    });
> +	}
>   
>   	me.advancedColumn1.push(
>   	    {
> @@ -372,14 +403,19 @@ Ext.define('PVE.qemu.HDEdit', {
>   	    confid: me.confid,
>   	    nodename: nodename,
>   	    unused: unused,
> -	    isCreate: me.isCreate
> +	    isCreate: me.isCreate,
> +	    isImport: me.isImport,
>   	});
>   
>   	var subject;
>   	if (unused) {
>   	    me.subject = gettext('Unused Disk');
> +	} else if (me.isImport) {
> +	    me.title = 'Import Hard Disk';

for consistency I would not change the title and instead set the following:
me.subject = gettext('Import Disk'),

> +	    me.submitText = 'Import';
> +	    me.backgroundDelay = undefined;
>   	} else if (me.isCreate) {
> -            me.subject = gettext('Hard Disk');
> +	    me.subject = gettext('Hard Disk');
>   	} else {
>              me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
>   	}
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 40b3fe86..f9735998 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -436,6 +436,23 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    handler: run_move
>   	});
>   
> +	var import_btn = new Proxmox.button.Button({
> +	    text: gettext('Import disk'),
> +	    hidden: !(caps.vms['VM.Allocate'] &&
> +		caps.storage['Datastore.AllocateTemplate'] &&
> +		caps.storage['Datastore.AllocateSpace']),
> +	    handler: function() {
> +		var win = Ext.create('PVE.qemu.HDEdit', {
> +		    method: 'POST',
> +		    url: `/api2/extjs/${baseurl}`,
> +		    pveSelNode: me.pveSelNode,
> +		    isImport: true,
> +		});
> +		win.on('destroy', me.reload, me);
> +		win.show();
> +	    },
> +	});
> +
>   	var remove_btn = new Proxmox.button.Button({
>   	    text: gettext('Remove'),
>   	    defaultText: gettext('Remove'),
> @@ -752,6 +769,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		edit_btn,
>   		resize_btn,
>   		move_btn,
> +		import_btn,
>   		revert_btn
>   	    ],
>   	    rows: rows,
>