Re: [pve-devel] [PATCH manager 4/5] ui: vm: make cloudinit drive editable

[Lukas Wagner <l.wagner@proxmox.com>]

Some suggestions inline. Skimmed over the code to spot style issues, correctness
was not really checked.

Same remarks regarding `var` vs `let` apply also to this patch.

On  2024-10-16 18:47, Daniel Kral wrote:
> Implements the functionality to allow subsequent changes to the
> CloudInit drive under the VM "Hardware" tab. This is needed to implement
> further changes in a future commit.
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  www/manager6/qemu/CIDriveEdit.js  | 100 ++++++++++++++++++++++--------
>  www/manager6/qemu/HardwareView.js |   4 +-
>  2 files changed, 77 insertions(+), 27 deletions(-)
> 
> diff --git a/www/manager6/qemu/CIDriveEdit.js b/www/manager6/qemu/CIDriveEdit.js
> index a9ca8bf1..0494f9c5 100644
> --- a/www/manager6/qemu/CIDriveEdit.js
> +++ b/www/manager6/qemu/CIDriveEdit.js
> @@ -4,28 +4,55 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
>  
>      insideWizard: false,
>  
> -    vmconfig: {}, // used to select usused disks
> +    vmconfig: {}, // used to select unused disks

Maybe split this typo fix into a separate commit?

>  
>      onGetValues: function(values) {
>  	var me = this;
>  
> -	var drive = {};
>  	var params = {};
> -	drive.file = values.hdstorage + ":cloudinit";
> -	drive.format = values.diskformat;
> -	params[values.controller + values.deviceid] = PVE.Parser.printQemuDrive(drive);
> +	var confid = me.confid || values.controller + values.deviceid;
> +
> +	// only set these when we create cloudinit files
> +	if (me.isCreate) {
> +	    me.drive.file = values.hdstorage + ":cloudinit";
> +	    me.drive.format = values.diskformat;
> +	}
> +
> +	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +
>  	return params;
>      },
>  
>      setNodename: function(nodename) {
>  	var me = this;
> +

Maybe do this in a separate commit if you
want to do a change like this to e.g. improve readability

>  	me.down('#hdstorage').setNodename(nodename);
>  	me.down('#hdimage').setStorage(undefined, nodename);
>      },
>  
>      setVMConfig: function(config) {
>  	var me = this;
> -	me.down('#drive').setVMConfig(config, 'cdrom');
> +
> +	let bussel = me.down('#drive');
> +	if (bussel) {
> +	    bussel.setVMConfig(config, 'cdrom');
> +	}
> +    },

bussel as a in ... bus selector? Maybe rather call it busSelector, has less of a
'huh?'-factor :)

> +
> +    setDrive: function(drive) {
> +	var me = this;
> +
> +	var values = {};
> +
> +	var match = drive.file.match(/^([^:]+):/);
> +	if (match) {
> +	    values.hdstorage = match[1];
> +	}
> +	values.hdimage = drive.file;
> +
> +	me.drive = drive;
> +
> +	me.setValues(values);
>      },
>  
>      initComponent: function() {
> @@ -33,22 +60,30 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
>  
>  	me.drive = {};
>  
> -	me.items = [
> -	    {
> +	let items = [];
> +
> +	if (me.isCreate) {
> +	    items.push({
>  		xtype: 'pveControllerSelector',
>  		withVirtIO: false,
>  		itemId: 'drive',
>  		fieldLabel: gettext('CloudInit Drive'),
>  		name: 'drive',
> -	    },
> -	    {
> -		xtype: 'pveDiskStorageSelector',
> -		itemId: 'storselector',
> -		storageContent: 'images',
> -		nodename: me.nodename,
> -		hideSize: true,
> -	    },
> -	];
> +	    });
> +	}
> +
> +	items.push({
> +	    xtype: 'pveDiskStorageSelector',
> +	    itemId: 'storselector',
> +	    storageContent: 'images',
> +	    nodename: me.nodename,
> +	    disabled: !me.isCreate,
> +	    hideFormat: !me.isCreate,
> +	    hideSize: true,
> +	});
> +
> +	me.items = items;
> +
>  	me.callParent();
>      },
>  });
> @@ -57,9 +92,6 @@ Ext.define('PVE.qemu.CIDriveEdit', {
>      extend: 'Proxmox.window.Edit',
>      xtype: 'pveCIDriveEdit',
>  
> -    isCreate: true,
> -    subject: gettext('CloudInit Drive'),
> -
>      initComponent: function() {
>  	var me = this;
>  
> @@ -68,17 +100,35 @@ Ext.define('PVE.qemu.CIDriveEdit', {
>  	    throw "no node name specified";
>  	}
>  
> -	me.items = [{
> -	    xtype: 'pveCIDriveInputPanel',
> -	    itemId: 'cipanel',
> +	me.isCreate = !me.confid;
> +
> +	var ipanel = Ext.create('PVE.qemu.CIDriveInputPanel', {
> +	    confid: me.confid,
>  	    nodename: nodename,
> -	}];
> +	    isCreate: me.isCreate,
> +	});
> +
> +	Ext.applyIf(me, {
> +	    subject: gettext('CloudInit Drive'),
> +	    items: [ipanel],
> +	});
>  
>  	me.callParent();
>  
>  	me.load({
>  	    success: function(response, opts) {
> -		me.down('#cipanel').setVMConfig(response.result.data);
> +		ipanel.setVMConfig(response.result.data);
> +
> +		if (me.confid) {
> +		    var value = response.result.data[me.confid];
> +		    var drive = PVE.Parser.parseQemuDrive(me.confid, value);
> +		    if (!drive) {
> +			Ext.Msg.alert('Error', 'Unable to parse drive options');
> +			me.close();
> +			return;
> +		    }
> +		    ipanel.setDrive(drive);
> +		}
>  	    },
>  	});
>      },
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 86d5f4cf..c7a77bd9 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -350,7 +350,7 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    if (rowdef.isOnStorageBus) {
>  		let value = me.getObjectValue(rec.data.key, '', true);
>  		if (isCloudInitKey(value)) {
> -		    return;
> +		    editor = 'PVE.qemu.CIDriveEdit';
>  		} else if (value.match(/media=cdrom/)) {
>  		    editor = 'PVE.qemu.CDEdit';
>  		} else if (!diskCap) {
> @@ -629,7 +629,7 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    remove_btn.RESTMethod = isUnusedDisk || (isDisk && isRunning) ? 'POST' : 'PUT';
>  
>  	    edit_btn.setDisabled(
> -	        deleted || !row.editor || isCloudInit || (isCDRom && !cdromCap) || (isDisk && !diskCap));
> +	        deleted || !row.editor || (isCDRom && !cdromCap) || (isDisk && !diskCap));
>  
>  	    diskaction_btn.setDisabled(
>  		pending ||

-- 
- Lukas


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel