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.koe...@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
