--env should be propagated to sudo commands

[w568w]

(I am posting this here instead of opening an Issue because it is open for discussion whether this is an unexpected bug, a documentation oversight, or a missing feature.)

Currently, the help text for --env simply describes it as "Set environment variables," which is somewhat oversimplified. In practice, --env applies variables to the current topgrade process, which are then inherited by its child processes.

However, it is well known that the sudo boundary does not preserve most environment variables by default, including those set via --env. Consequently, for commands that require both sudo (e.g., apt) and network proxies (e.g., https_proxy="..."), Topgrade currently lacks a way to pass these proxy variables through to the underlying command (e.g., apt).

There are likely three ways to address this:

1. Treat it as a bug: Add the contents of the --env argument to preserve_env_list() and pass them to SudoExecuteOpts.
2. Treat it as a documentation oversight: Explicitly state in the help text that --env is not guaranteed to be passed to commands requiring sudo.
3. Treat it as a feature request: Introduce new flags (e.g., --sudo-env or --sudo-preserve-env-type) that allow users to specify additional variables or configure how variables are preserved across the sudo boundary.

What do you think?

[GideonBear]

I think we should treat this as a bug, I cannot come up with any reason why you would not want --env to be passed through sudo. (If you or someone else does, please say so!)

Notable details:

1. These need to apply everywhere (default SudoExecuteOps, maybe?), and not be overwritten by preserve_env_list (not necessary for preserve_env).
2. What if the same environment variable is explicitly set, e.g. sudo.execute_opts(..., SudoExecuteOpts::new().preserve_env_list(&["EXAMPLE"])).env("EXAMPLE", "some_value").status_checked() ? This currently happens nowhere, but I still think it should be considered which should take precedence then.

[w568w]

These need to apply everywhere (default SudoExecuteOps, maybe?), and not be overwritten by preserve_env_list (not necessary for preserve_env).

What if the same environment variable is explicitly set, [...]? This currently happens nowhere, but I still think it should be considered which should take precedence then.

My thought is that user settings should override the preserve_env_list in the code. Perhaps a better approach would be to use layered settings and merge them later:

#[derive(Clone, Debug, Default)]
pub struct SudoExecuteOpts<'a> {
    /// Run the command inside a login shell.
    pub login_shell: bool,
    /// Preserve environment variables across the sudo call.
    pub preserve_env: SudoPreserveEnv<'a>,
+   /// Preserved environment variables set via --env arguments by user, overriding preserve_env.
+   pub user_preserve_env: SudoPreserveEnv<'a>,
...

pub fn execute_opts<S: AsRef<OsStr>>(
...
-   match opts.preserve_env {
+   match opts.merged_preserve_env() { // add a method to merge the two settings
...

[GideonBear]

On second thought, I don't think giving the user-specified env priority makes a lot of sense; a non-sudo explicitly set env var would currently override any user-specified env (right?). That would make adding sudo to a command potentially change the behavior.

[w568w]

a non-sudo explicitly set env var would currently override any user-specified env (right?). That would make adding sudo to a command potentially change the behavior.

Indeed, that's also reasonable. I personally don't have a strong preference regarding the order here; the key is that this precedence should be explicitly stated in the help text or documentation to avoid confusion.

[GideonBear]

Opened #1588. @AThePeanut4 does this make sense?