Am 29/01/2024 um 16:43 schrieb Folke Gleumes: > The keep-env option allows the user to define if the current environment > should be kept when running 'pct enter/exec'. pct will now always set > '--keep-env' or '--discard-env' when calling lxc-attach to anticipate > the upcoming change in default behavior.
seems fine in general, but I saw a two code style nits, of which one I'd really like to avoid, see comment in line. > > Signed-off-by: Folke Gleumes <f.gleu...@proxmox.com> > --- > This wasn't present in v1 > > src/PVE/CLI/pct.pm | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm > index 091ac8e..7ce0de0 100755 > --- a/src/PVE/CLI/pct.pm > +++ b/src/PVE/CLI/pct.pm > @@ -162,12 +162,21 @@ __PACKAGE__->register_method ({ > additionalProperties => 0, > properties => { > vmid => get_standard_option('pve-vmid', { completion => > \&PVE::LXC::complete_ctid_running }), > + # TODO: set keep-env to default false with PVE 9 no hard feelings for thiss one, but could be a better fit if located in the default line, which would be the one that will be changed, e.g.: default => 1, # TODO: make keep-env opt-out here and in the code with PVE 9 or before with even more context, something like: # FIXME: passing the environment in CT is really not ideal, it can leak secrets or cause # programs to do behave weird, so change to opt-in in PVE 9 Having some more context about the reasoning can be good for such comments (or at least the commit message), even if it should be relatively clear, like here. > + 'keep-env' => {> + type => 'boolean', > + description => "Keep the current environment. This option will > disabled by default with PVE 9. " nit: for line continuations the next line should hold the space, that way one can add or remove sentences more likely without touching other unrelated lines. As example for here: description => "Keep the current environment. This option will disabled by default with PVE 9." ." If you rely on a preserved environment, please use this option to be future-proof." But would not have blocked applying just because of this, so mostly mentioning it due to writing a reply anyway. > + ."If you rely on a preserved environment, please use this > option to be future-proof.", > @@ -175,7 +184,7 @@ __PACKAGE__->register_method ({ > die "container '$vmid' not running!\n" if > !PVE::LXC::check_running($vmid); > > clean_environment(); > - exec('lxc-attach', '-n', $vmid); > + exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', > $vmid); I'd prefer to avoid such "hidden" ternaries inside other statements, that makes the code unnecessarily hard to read without much gain. While it makes it longer, having this split over a few line seems a bit nicer, e.g. something like (untested): my @lxc_attach_cmd = ('lxc-attach', '-n', $vmid); push @lxc_attach_cmd, $keep_env ? '--keep-env' : '--clear-env'; exec(@lxc_attach_cmd); > }}); > > __PACKAGE__->register_method ({ > @@ -187,12 +196,21 @@ __PACKAGE__->register_method ({ > additionalProperties => 0, > properties => { > vmid => get_standard_option('pve-vmid', { completion => > \&PVE::LXC::complete_ctid_running }), > + # TODO: set keep-env to default false with PVE 9 > + 'keep-env' => { > + type => 'boolean', > + description => "Keep the current environment. This option will > disabled by default with PVE 9. " same here w.r.t. trailing space for literal string continuation > + ."If you rely on a preserved environment, please use this > option to be future-proof.", > + optional => 1, > + default => 1, > + }, > 'extra-args' => get_standard_option('extra-args'), > }, > }, > returns => { type => 'null' }, > code => sub { > my ($param) = @_; > + my $keep_env = $param->{'keep-env'} // 1; # TODO: toggle with pve 9 > > my $vmid = $param->{vmid}; > PVE::LXC::Config->load_config($vmid); # test if container exists on > this node > @@ -201,7 +219,7 @@ __PACKAGE__->register_method ({ > die "missing command" if !@{$param->{'extra-args'}}; > > clean_environment(); > - exec('lxc-attach', '-n', $vmid, '--', @{$param->{'extra-args'}}); > + exec('lxc-attach', $keep_env ? '--keep-env' : '--clear-env', '-n', > $vmid, '--', @{$param->{'extra-args'}}); same here w.r.t. ternary, gets a bit unwieldy > }}); > > __PACKAGE__->register_method ({ _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel