Re: [pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation

["Michael Köppl" <m.koeppl@proxmox.com>]

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