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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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