* [pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation @ 2025-03-18 16:14 Michael Köppl 2025-03-21 7:58 ` Thomas Lamprecht 0 siblings, 1 reply; 3+ messages in thread From: Michael Köppl @ 2025-03-18 16:14 UTC (permalink / raw) To: pve-devel The current implementation is slightly misleading. When creating a privileged container, the nesting checkbox is disabled but keeps its current state. However, nesting is not enabled for privileged containers even if the checkbox was set to true. Enabling nesting is still possible through the Options menu. Signed-off-by: Michael Köppl <m.koeppl@proxmox.com> --- As an alternative to this, since we already discourage the use of privileged containers [0], removing the checkbox for creating privileged containers in the web UI might make sense. For the rare cases where they are required, they can still be created using pct (although the question remains whether privileged should be the default for pct create). [0] https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850 www/manager6/lxc/CreateWizard.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js index 62cda27a..c7ee56f7 100644 --- a/www/manager6/lxc/CreateWizard.js +++ b/www/manager6/lxc/CreateWizard.js @@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', { nodename: '', storage: '', unprivileged: true, + nestingEnabled: true, }, formulas: { cgroupMode: function(get) { @@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', { value: '{unprivileged}', }, fieldLabel: gettext('Unprivileged container'), + listeners: { + change: function(checkbox, value) { + var viewModel = checkbox.lookupViewModel(); + if (!value && viewModel) { + viewModel.set('nestingEnabled', false); + } + }, + }, }, { xtype: 'proxmoxcheckbox', name: 'features', inputValue: 'nesting=1', - value: true, bind: { disabled: '{!unprivileged}', + value: '{nestingEnabled}', }, fieldLabel: gettext('Nesting'), }, -- 2.39.5 _______________________________________________ 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] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation 2025-03-18 16:14 [pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation Michael Köppl @ 2025-03-21 7:58 ` Thomas Lamprecht 2025-03-21 10:38 ` Michael Köppl 0 siblings, 1 reply; 3+ messages in thread From: Thomas Lamprecht @ 2025-03-21 7:58 UTC (permalink / raw) To: Proxmox VE development discussion, Michael Köppl Thanks for this patch, some comments inline. Am 18.03.25 um 17:14 schrieb Michael Köppl: > The current implementation is slightly misleading. When creating a > privileged container, the nesting checkbox is disabled but keeps its > current state. However, nesting is not enabled for privileged containers > even if the checkbox was set to true. Enabling nesting is still possible > through the Options menu. > > Signed-off-by: Michael Köppl <m.koeppl@proxmox.com> > --- > As an alternative to this, since we already discourage > the use of privileged containers [0], removing the checkbox for creating privileged > containers in the web UI might make sense. For the rare cases where they > are required, they can still be created using pct (although the question > remains whether privileged should be the default for pct create). > > [0] https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850 > > www/manager6/lxc/CreateWizard.js | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js > index 62cda27a..c7ee56f7 100644 > --- a/www/manager6/lxc/CreateWizard.js > +++ b/www/manager6/lxc/CreateWizard.js > @@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', { > nodename: '', > storage: '', > unprivileged: true, > + nestingEnabled: true, This works fine as is, but I think there is a better solution available here (see below) and such use of viewModel data binding can come back and bite one when extending/refactoring such code, as the code using it needs to be careful to not interfere with the automatic two-way binding to the field's value. I do not want to discourage using the viewModel, most of the time it can be fine and result in even nicer code overall, but it has its oddities. > }, > formulas: { > cgroupMode: function(get) { > @@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', { > value: '{unprivileged}', > }, > fieldLabel: gettext('Unprivileged container'), > + listeners: { > + change: function(checkbox, value) { > + var viewModel = checkbox.lookupViewModel(); style nit: we mainly use `let` variables for new code as that has less confusing scope rules, i.e. variables declared through var are also available in the parent block scope. > + if (!value && viewModel) { > + viewModel.set('nestingEnabled', false); You could also avoid the intermediate viewModel variable that is used just once and use an optional-chaining operator, like: checkbox.lookupViewModel()?.set('nestingEnabled', false); > + } > + }, > + }, > }, > { > xtype: 'proxmoxcheckbox', FYI that component comes from our proxmox-widget-toolkit and is a small override of the original ExtJS checkbox. One thing it provides is a `clearOnDisable` config flag that if set to true should exactly provide the behavior you want here without needing to hook into the unprivileged change listener. See src/form/Checkbox.js in the proxmox-widget-toolkit for the rather trivial implementation of that flag. > name: 'features', > inputValue: 'nesting=1', > - value: true, > bind: { > disabled: '{!unprivileged}', > + value: '{nestingEnabled}', > }, > fieldLabel: gettext('Nesting'), > }, _______________________________________________ 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] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation 2025-03-21 7:58 ` Thomas Lamprecht @ 2025-03-21 10:38 ` Michael Köppl 0 siblings, 0 replies; 3+ messages in thread From: Michael Köppl @ 2025-03-21 10:38 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion Superseded byhttps://lore.proxmox.com/pve-devel/20250321103511.66722-1-m.koeppl@proxmox.com/ Thank you for having a look and your detailed suggestions! > style nit: we mainly use `let` variables for new code as that has less confusing > scope rules, i.e. variables declared through var are also available in the parent > block scope. Also thanks for this hint. Makes sense, will adhere to it for future patches. On 3/21/25 08:58, Thomas Lamprecht wrote: > Thanks for this patch, some comments inline. > > Am 18.03.25 um 17:14 schrieb Michael Köppl: >> The current implementation is slightly misleading. When creating a >> privileged container, the nesting checkbox is disabled but keeps its >> current state. However, nesting is not enabled for privileged containers >> even if the checkbox was set to true. Enabling nesting is still possible >> through the Options menu. >> >> Signed-off-by: Michael Köppl<m.koeppl@proxmox.com> >> --- >> As an alternative to this, since we already discourage >> the use of privileged containers [0], removing the checkbox for creating privileged >> containers in the web UI might make sense. For the rare cases where they >> are required, they can still be created using pct (although the question >> remains whether privileged should be the default for pct create). >> >> [0]https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850 >> >> www/manager6/lxc/CreateWizard.js | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js >> index 62cda27a..c7ee56f7 100644 >> --- a/www/manager6/lxc/CreateWizard.js >> +++ b/www/manager6/lxc/CreateWizard.js >> @@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', { >> nodename: '', >> storage: '', >> unprivileged: true, >> + nestingEnabled: true, > This works fine as is, but I think there is a better solution available here > (see below) and such use of viewModel data binding can come back and bite one > when extending/refactoring such code, as the code using it needs to be careful > to not interfere with the automatic two-way binding to the field's value. > I do not want to discourage using the viewModel, most of the time it can be > fine and result in even nicer code overall, but it has its oddities. > >> }, >> formulas: { >> cgroupMode: function(get) { >> @@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', { >> value: '{unprivileged}', >> }, >> fieldLabel: gettext('Unprivileged container'), >> + listeners: { >> + change: function(checkbox, value) { >> + var viewModel = checkbox.lookupViewModel(); > style nit: we mainly use `let` variables for new code as that has less confusing > scope rules, i.e. variables declared through var are also available in the parent > block scope. > >> + if (!value && viewModel) { >> + viewModel.set('nestingEnabled', false); > You could also avoid the intermediate viewModel variable that is used just once > and use an optional-chaining operator, like: > > checkbox.lookupViewModel()?.set('nestingEnabled', false); > >> + } >> + }, >> + }, >> }, >> { >> xtype: 'proxmoxcheckbox', > FYI that component comes from our proxmox-widget-toolkit and is a small override > of the original ExtJS checkbox. One thing it provides is a `clearOnDisable` > config flag that if set to true should exactly provide the behavior you want > here without needing to hook into the unprivileged change listener. > > See src/form/Checkbox.js in the proxmox-widget-toolkit for the rather trivial > implementation of that flag. > >> name: 'features', >> inputValue: 'nesting=1', >> - value: true, >> bind: { >> disabled: '{!unprivileged}', >> + value: '{nestingEnabled}', >> }, >> fieldLabel: gettext('Nesting'), >> }, _______________________________________________ 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-03-21 10:39 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-03-18 16:14 [pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation Michael Köppl 2025-03-21 7:58 ` Thomas Lamprecht 2025-03-21 10:38 ` Michael Köppl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inboxService provided by Proxmox Server Solutions GmbH | Privacy | Legal