From: Lukas Wagner <l.wagner@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Daniel Kral <d.kral@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager 4/5] ui: vm: make cloudinit drive editable
Date: Fri, 18 Oct 2024 09:42:24 +0200 [thread overview]
Message-ID: <8e603ada-fc40-4a54-8121-96c83c1c6511@proxmox.com> (raw)
In-Reply-To: <20241016164711.934544-5-d.kral@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
next prev parent reply other threads:[~2024-10-18 7:41 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-16 16:47 [pve-devel] [PATCH kernel/manager 0/5] fix #5430: ocfs2 io_uring read Daniel Kral
2024-10-16 16:47 ` [pve-devel] [PATCH kernel 1/5] fix #5430: cherry-pick fix for ocfs2 io_uring rw issues Daniel Kral
2024-10-17 14:48 ` Friedrich Weber
2024-10-22 14:03 ` [pve-devel] applied: " Thomas Lamprecht
2024-10-16 16:47 ` [pve-devel] [PATCH manager 2/5] ui: vm: factor out async I/O type selector Daniel Kral
2024-10-18 7:42 ` Lukas Wagner
2024-10-16 16:47 ` [pve-devel] [PATCH manager 3/5] fix #5430: ui: vm: allow editing cdrom aio and cache options Daniel Kral
2024-10-18 7:42 ` Lukas Wagner
2024-10-23 11:30 ` Daniel Kral
2024-10-16 16:47 ` [pve-devel] [PATCH manager 4/5] ui: vm: make cloudinit drive editable Daniel Kral
2024-10-18 7:42 ` Lukas Wagner [this message]
2024-10-16 16:47 ` [pve-devel] [PATCH manager 5/5] fix #5430: ui: vm: allow editing cloudinit aio and cache options Daniel Kral
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e603ada-fc40-4a54-8121-96c83c1c6511@proxmox.com \
--to=l.wagner@proxmox.com \
--cc=d.kral@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox