[pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation
Michael Köppl
m.koeppl at proxmox.com
Fri Mar 21 11:38:25 CET 2025
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 at 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'),
>> },
More information about the pve-devel
mailing list