Thanks for the review!
On Fri, Jan 26, 2024 at 03:47:34PM +0100, Fiona Ebner wrote:
> Am 21.11.23 um 10:40 schrieb Christoph Heiss:
[..]
> > diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
> > index c702f3d..7f23111 100644
> > --- a/src/PVE/LXC/Setup/NixOS.pm
> > +++ b/src/PVE/LXC/Setup/NixOS.pm
> > @@ -17,6 +17,11 @@ sub new {
> >
> > $conf->{ostype} = "nixos";
> >
> > + # Set `cmode` to `console` for NixOS containers, as getty is only
> > configured for /dev/console by
> > + # default, but not any TTY ports. This way, users still get a
> > login/shell instead of just a
> > + # blank screen when openinng the console in the web UI.
> > + $conf->{cmode} = 'console';
> > +
> > return bless $self, $class;
> > }
> >
>
> Won't this override any pre-existing setting (from user or backup)?
> Even if checking for that, it could still be considered a breaking
> change, i.e. other people might have configured their NixOS container
> differently, so maybe best to wait for the next major release?
Yep, just tried that out - so that's definitely a no-go.
>
> For now, we could log a warning if no explicit 'cmode' was specified for
> NixOS.
>
> I don't see any other implementation of new() overriding any defaults.
> Should we even start doing that?
Well, looking at the code again, there is the ->template_fixup() sub,
which gets called on restores after ->new(). This does seem to override
some things (e.g. ->setup_securetty()) for most of the distros.
So I guess this would be correct place after all for this, if we
override something.
Anyway, so just a
warn "..." if `$conf->{cmode} ne 'console'`;
if ->new() would be acceptable?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
