From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Folke Gleumes <f.gleumes@proxmox.com>
Subject: [pve-devel] applied: [PATCH container v2 2/2] pct: add keep-env option
Date: Fri, 9 Feb 2024 20:07:36 +0100 [thread overview]
Message-ID: <5ec111f0-d051-412d-887b-e42f2bf97013@proxmox.com> (raw)
In-Reply-To: <9e65cabe-9251-4b3c-b8ad-c761df5cca0a@proxmox.com>
Am 02/02/2024 um 17:53 schrieb Thomas Lamprecht:
> 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.gleumes@proxmox.com>
>> ---
>> This wasn't present in v1
>>
>> src/PVE/CLI/pct.pm | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>>
applied, thanks!
Two small things though:
- there where two instances with double whitespace, e.g. after the
`my @lxc_attach_cmd = `and before `$vmid`, and that then copied over
to the other command too. It really did not seemed to be due to
some vertical alignment, so I ameded the commit and dropped them.
- in general it's preferred to send new patches, single or series, as new
thread, i.e., without in-reply too. I know some open source projects
sometimes are fine with sending those as in-reply-to, but IMO that
hides them more. It's naturally fine to propose ad-hock diffs/code
in a discussion in thread, but if there's a full patch with a new
revision, then one is basically always better off when sending that
as new thread.
Oh, and while at it, w.r.t perl array-refs you nowadays can write
`@{$foo->{bar}}` as `$foo->{bar}->@*`, and you can concat lists,
so the final "add base command + user params then exec" part could
look like:
exec(@lxc_attach_cmd, '--', $foo->{bar}->@*);
I kept that as is for now, it's IMO really not worse if done explicitly.
next prev parent reply other threads:[~2024-02-09 19:07 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-29 15:43 [pve-devel] [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Folke Gleumes
2024-01-29 15:43 ` [pve-devel] [PATCH container v2 2/2] pct: add keep-env option Folke Gleumes
2024-02-02 16:53 ` Thomas Lamprecht
2024-02-09 13:17 ` [pve-devel] [PATCH container v3] " Folke Gleumes
2024-02-09 19:07 ` Thomas Lamprecht [this message]
2024-02-02 16:32 ` [pve-devel] applied: [PATCH container v2 1/2] fix #5194: pct: delete environment variables set by pve Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5ec111f0-d051-412d-887b-e42f2bf97013@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=f.gleumes@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox