* [pve-devel] [PATCH manager 1/1] fix: ha: set ha-managed param default and unchecked value to undefined @ 2025-11-17 13:18 Michael Köppl 2025-11-17 16:47 ` Thomas Lamprecht 0 siblings, 1 reply; 3+ messages in thread From: Michael Köppl @ 2025-11-17 13:18 UTC (permalink / raw) To: pve-devel This avoids sending the parameter with the default value when it's not necessary, since the API already defines the param as optional and assigns a default value of 0. Signed-off-by: Michael Köppl <m.koeppl@proxmox.com> --- Thanks to @Friedrich for bringing this to my attention. A case where this occurs is in clusters where at least one of the nodes does not yet support the ha-managed param and a VM is created on that node from the web UI of a node that already supports it. Although in general it can probably be expected that all nodes should be upgraded to a new version, this avoids failing the CreateWizard dialog entirely for cases where the "Add to HA" checkbox wasn't even checked at basically no cost because the API already defines a default value of 0 and considers the param optional. www/manager6/lxc/CreateWizard.js | 4 ++-- www/manager6/qemu/CreateWizard.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js index f35709f14..024536c58 100644 --- a/www/manager6/lxc/CreateWizard.js +++ b/www/manager6/lxc/CreateWizard.js @@ -85,8 +85,8 @@ Ext.define('PVE.lxc.CreateWizard', { { xtype: 'proxmoxcheckbox', name: 'ha-managed', - uncheckedValue: 0, - defaultValue: 0, + uncheckedValue: undefined, + defaultValue: undefined, fieldLabel: gettext('Add to HA'), }, ], diff --git a/www/manager6/qemu/CreateWizard.js b/www/manager6/qemu/CreateWizard.js index e0c56bc0b..564ca1176 100644 --- a/www/manager6/qemu/CreateWizard.js +++ b/www/manager6/qemu/CreateWizard.js @@ -99,8 +99,8 @@ Ext.define('PVE.qemu.CreateWizard', { { xtype: 'proxmoxcheckbox', name: 'ha-managed', - uncheckedValue: 0, - defaultValue: 0, + uncheckedValue: undefined, + defaultValue: undefined, fieldLabel: gettext('Add to HA'), }, ], -- 2.47.3 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH manager 1/1] fix: ha: set ha-managed param default and unchecked value to undefined 2025-11-17 13:18 [pve-devel] [PATCH manager 1/1] fix: ha: set ha-managed param default and unchecked value to undefined Michael Köppl @ 2025-11-17 16:47 ` Thomas Lamprecht 2025-11-17 17:35 ` Michael Köppl 0 siblings, 1 reply; 3+ messages in thread From: Thomas Lamprecht @ 2025-11-17 16:47 UTC (permalink / raw) To: Proxmox VE development discussion, Michael Köppl The commit subject could be a bit improved, as of now we do not use plain "fix" commits, besides that it affects the ui subsystem, which is a major one in this repo, so naming it makes sense, then it's not really for ha itself but rather for the (guest) create wizard, and finally, the commit message and it's subject should focus on what's fixed and ideally (in short) why thefix works and was chosen rather than describing how it is done, especially not if that's just then a rather literal description of the actual diff itself. E.g. here the following would be a lot clearer to me: "ui: create wizard: only submit ha-managed if checked for better compat" Am 17.11.25 um 14:17 schrieb Michael Köppl: > This avoids sending the parameter with the default value when it's not > necessary, since the API already defines the param as optional and > assigns a default value of 0. Why does this work though, 0 and undefined seem close enough for anybody without in-depth knowledge about ExtJS and/or our widget-toolkit. Would be nice to reference upstream docs for chechbox's uncheckedValue config parameter, which plays the core part here: > By default this is undefined, which results in nothing being submitted > for the checkbox field when the form is submitted -- https://docs.sencha.com/extjs/7.0.0/classic/Ext.form.field.Checkbox.html#cfg-uncheckedValue > > Signed-off-by: Michael Köppl <m.koeppl@proxmox.com> > --- > Thanks to @Friedrich for bringing this to my attention. A case where > this occurs is in clusters where at least one of the nodes does not yet > support the ha-managed param and a VM is created on that node from the > web UI of a node that already supports it. Although in general it can > probably be expected that all nodes should be upgraded to a new version, > this avoids failing the CreateWizard dialog entirely for cases where the > "Add to HA" checkbox wasn't even checked at basically no cost because > the API already defines a default value of 0 and considers the param > optional. > > www/manager6/lxc/CreateWizard.js | 4 ++-- > www/manager6/qemu/CreateWizard.js | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js > index f35709f14..024536c58 100644 > --- a/www/manager6/lxc/CreateWizard.js > +++ b/www/manager6/lxc/CreateWizard.js > @@ -85,8 +85,8 @@ Ext.define('PVE.lxc.CreateWizard', { > { > xtype: 'proxmoxcheckbox', > name: 'ha-managed', > - uncheckedValue: 0, > - defaultValue: 0, > + uncheckedValue: undefined, > + defaultValue: undefined, Both are the default for (proxmox)checkbox, so could be just omitted? Or if, then I'd just set "uncheckedValue" explicitly and add a small comment there so that it's a bit more explicit and telling. > fieldLabel: gettext('Add to HA'), > }, > ], > diff --git a/www/manager6/qemu/CreateWizard.js b/www/manager6/qemu/CreateWizard.js > index e0c56bc0b..564ca1176 100644 > --- a/www/manager6/qemu/CreateWizard.js > +++ b/www/manager6/qemu/CreateWizard.js > @@ -99,8 +99,8 @@ Ext.define('PVE.qemu.CreateWizard', { > { > xtype: 'proxmoxcheckbox', > name: 'ha-managed', > - uncheckedValue: 0, > - defaultValue: 0, > + uncheckedValue: undefined, > + defaultValue: undefined, Same here. > fieldLabel: gettext('Add to HA'), > }, > ], _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH manager 1/1] fix: ha: set ha-managed param default and unchecked value to undefined 2025-11-17 16:47 ` Thomas Lamprecht @ 2025-11-17 17:35 ` Michael Köppl 0 siblings, 0 replies; 3+ messages in thread From: Michael Köppl @ 2025-11-17 17:35 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion, Michael Köppl Thanks for the detailed feedback! Sent a v2 with a better commit message and only uncheckedValue set explicitly. https://lore.proxmox.com/pve-devel/20251117173321.240894-1-m.koeppl@proxmox.com On Mon Nov 17, 2025 at 5:47 PM CET, Thomas Lamprecht wrote: > The commit subject could be a bit improved, as of now we do not use plain > "fix" commits, besides that it affects the ui subsystem, which is a major > one in this repo, so naming it makes sense, then it's not really for ha > itself but rather for the (guest) create wizard, and finally, the commit > message and it's subject should focus on what's fixed and ideally (in short) > why thefix works and was chosen rather than describing how it is done, > especially not if that's just then a rather literal description of the > actual diff itself. > > E.g. here the following would be a lot clearer to me: > > "ui: create wizard: only submit ha-managed if checked for better compat" > > > Am 17.11.25 um 14:17 schrieb Michael Köppl: >> This avoids sending the parameter with the default value when it's not >> necessary, since the API already defines the param as optional and >> assigns a default value of 0. > > Why does this work though, 0 and undefined seem close enough for anybody > without in-depth knowledge about ExtJS and/or our widget-toolkit. > > Would be nice to reference upstream docs for chechbox's uncheckedValue > config parameter, which plays the core part here: > >> By default this is undefined, which results in nothing being submitted >> for the checkbox field when the form is submitted > > -- https://docs.sencha.com/extjs/7.0.0/classic/Ext.form.field.Checkbox.html#cfg-uncheckedValue > >> >> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com> >> --- >> Thanks to @Friedrich for bringing this to my attention. A case where >> this occurs is in clusters where at least one of the nodes does not yet >> support the ha-managed param and a VM is created on that node from the >> web UI of a node that already supports it. Although in general it can >> probably be expected that all nodes should be upgraded to a new version, >> this avoids failing the CreateWizard dialog entirely for cases where the >> "Add to HA" checkbox wasn't even checked at basically no cost because >> the API already defines a default value of 0 and considers the param >> optional. >> >> www/manager6/lxc/CreateWizard.js | 4 ++-- >> www/manager6/qemu/CreateWizard.js | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js >> index f35709f14..024536c58 100644 >> --- a/www/manager6/lxc/CreateWizard.js >> +++ b/www/manager6/lxc/CreateWizard.js >> @@ -85,8 +85,8 @@ Ext.define('PVE.lxc.CreateWizard', { >> { >> xtype: 'proxmoxcheckbox', >> name: 'ha-managed', >> - uncheckedValue: 0, >> - defaultValue: 0, >> + uncheckedValue: undefined, >> + defaultValue: undefined, > > Both are the default for (proxmox)checkbox, so could be just omitted? Or if, then > I'd just set "uncheckedValue" explicitly and add a small comment there so that it's > a bit more explicit and telling. > >> fieldLabel: gettext('Add to HA'), >> }, >> ], >> diff --git a/www/manager6/qemu/CreateWizard.js b/www/manager6/qemu/CreateWizard.js >> index e0c56bc0b..564ca1176 100644 >> --- a/www/manager6/qemu/CreateWizard.js >> +++ b/www/manager6/qemu/CreateWizard.js >> @@ -99,8 +99,8 @@ Ext.define('PVE.qemu.CreateWizard', { >> { >> xtype: 'proxmoxcheckbox', >> name: 'ha-managed', >> - uncheckedValue: 0, >> - defaultValue: 0, >> + uncheckedValue: undefined, >> + defaultValue: undefined, > > Same here. > >> fieldLabel: gettext('Add to HA'), >> }, >> ], _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-11-17 17:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-11-17 13:18 [pve-devel] [PATCH manager 1/1] fix: ha: set ha-managed param default and unchecked value to undefined Michael Köppl 2025-11-17 16:47 ` Thomas Lamprecht 2025-11-17 17:35 ` Michael Köppl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox