refactor(subp): Remove redundant parameter 'env' (#4555)

- Remove redundant code from the few callsites that used this parameter.
- Eliminate tests for behavior that was never used.

cloudinit/config/cc_ansible.py

@@ -70,12 +70,12 @@ def __init__(self, distro: Distro):
         self.cmd_pull = ["ansible-pull"]
         self.cmd_version = ["ansible-pull", "--version"]
         self.distro = distro
-        self.env = os.environ
+        self.env = {}
         self.run_user: Optional[str] = None
 
         # some ansible modules directly reference os.environ["HOME"]
         # and cloud-init might not have that set, default: /root
-        self.env["HOME"] = self.env.get("HOME", "/root")
+        self.env["HOME"] = os.environ.get("HOME", "/root")
 
     def get_version(self) -> Optional[Version]:
         stdout, _ = self.do_as(self.cmd_version)
@@ -100,7 +100,7 @@ def do_as(self, command: list, **kwargs):
         return self.distro.do_as(command, self.run_user, **kwargs)
 
     def subp(self, command, **kwargs):
-        return subp(command, env=self.env, **kwargs)
+        return subp(command, update_env=self.env, **kwargs)
 
     @abc.abstractmethod
     def is_installed(self):

cloudinit/config/cc_bootcmd.py

@@ -10,7 +10,6 @@
 """Bootcmd: run arbitrary commands early in the boot process."""
 
 import logging
-import os
 from textwrap import dedent
 
 from cloudinit import subp, temp_utils, util
@@ -83,12 +82,9 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
             raise
 
         try:
-            env = os.environ.copy()
             iid = cloud.get_instance_id()
-            if iid:
-                env["INSTANCE_ID"] = str(iid)
-            cmd = ["/bin/sh", tmpf.name]
-            subp.subp(cmd, env=env, capture=False)
+            env = {"INSTANCE_ID": str(iid)} if iid else {}
+            subp.subp(["/bin/sh", tmpf.name], update_env=env, capture=False)
         except Exception:
             util.logexc(LOG, "Failed to run bootcmd module %s", name)
             raise

cloudinit/config/cc_growpart.py

@@ -167,11 +167,10 @@ def resize(self, diskdev, partnum, partdev):
 
 class ResizeGrowPart(Resizer):
     def available(self, devices: list):
-        myenv = os.environ.copy()
-        myenv["LANG"] = "C"
-
         try:
-            (out, _err) = subp.subp(["growpart", "--help"], env=myenv)
+            out = subp.subp(
+                ["growpart", "--help"], update_env={"LANG": "C"}
+            ).stdout
             if re.search(r"--update\s+", out):
                 return True
 
@@ -180,21 +179,20 @@ def available(self, devices: list):
         return False
 
     def resize(self, diskdev, partnum, partdev):
-        myenv = os.environ.copy()
-        myenv["LANG"] = "C"
         before = get_size(partdev)
 
         # growpart uses tmp dir to store intermediate states
         # and may conflict with systemd-tmpfiles-clean
         tmp_dir = self._distro.get_tmp_exec_path()
         with temp_utils.tempdir(dir=tmp_dir, needs_exe=True) as tmpd:
             growpart_tmp = os.path.join(tmpd, "growpart")
+            my_env = {"LANG": "C", "TMPDIR": growpart_tmp}
             if not os.path.exists(growpart_tmp):
                 os.mkdir(growpart_tmp, 0o700)
-            myenv["TMPDIR"] = growpart_tmp
             try:
                 subp.subp(
-                    ["growpart", "--dry-run", diskdev, partnum], env=myenv
+                    ["growpart", "--dry-run", diskdev, partnum],
+                    update_env=my_env,
                 )
             except subp.ProcessExecutionError as e:
                 if e.exit_code != 1:
@@ -208,7 +206,7 @@ def resize(self, diskdev, partnum, partdev):
                 return (before, before)
 
             try:
-                subp.subp(["growpart", diskdev, partnum], env=myenv)
+                subp.subp(["growpart", diskdev, partnum], update_env=my_env)
             except subp.ProcessExecutionError as e:
                 util.logexc(LOG, "Failed: growpart %s %s", diskdev, partnum)
                 raise ResizeFailedException(e) from e
@@ -246,11 +244,10 @@ def resize(self, diskdev, partnum, partdev):
 
 class ResizeGpart(Resizer):
     def available(self, devices: list):
-        myenv = os.environ.copy()
-        myenv["LANG"] = "C"
-
         try:
-            (_out, err) = subp.subp(["gpart", "help"], env=myenv, rcs=[0, 1])
+            err = subp.subp(
+                ["gpart", "help"], update_env={"LANG": "C"}, rcs=[0, 1]
+            ).stderr
             if re.search(r"gpart recover ", err):
                 return True
 

cloudinit/config/cc_seed_random.py

@@ -10,7 +10,6 @@
 
 import base64
 import logging
-import os
 from io import BytesIO
 from textwrap import dedent
 
@@ -90,7 +89,7 @@ def _decode(data, encoding=None):
         raise IOError("Unknown random_seed encoding: %s" % (encoding))
 
 
-def handle_random_seed_command(command, required, env=None):
+def handle_random_seed_command(command, required, update_env):
     if not command and required:
         raise ValueError("no command found but required=true")
     elif not command:
@@ -106,7 +105,7 @@ def handle_random_seed_command(command, required, env=None):
         else:
             LOG.debug("command '%s' not found for seed_command", cmd)
             return
-    subp.subp(command, env=env, capture=False)
+    subp.subp(command, update_env=update_env, capture=False)
 
 
 def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
@@ -137,9 +136,11 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
     command = mycfg.get("command", None)
     req = mycfg.get("command_required", False)
     try:
-        env = os.environ.copy()
-        env["RANDOM_SEED_FILE"] = seed_path
-        handle_random_seed_command(command=command, required=req, env=env)
+        handle_random_seed_command(
+            command=command, required=req, update_env={
+                "RANDOM_SEED_FILE": seed_path
+            }
+        )
     except ValueError as e:
         LOG.warning("handling random command [%s] failed: %s", command, e)
         raise e

cloudinit/config/cc_snap.py

@@ -173,7 +173,7 @@ def run_commands(commands):
     for command in fixed_snap_commands:
         shell = isinstance(command, str)
         try:
-            subp.subp(command, shell=shell)
+            subp.subp([command], shell=shell)
         except subp.ProcessExecutionError as e:
             cmd_failures.append(str(e))
     if cmd_failures:

cloudinit/config/cc_ssh.py

@@ -275,8 +275,6 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
                 ",".join(skipped_keys),
             )
 
-        lang_c = os.environ.copy()
-        lang_c["LANG"] = "C"
         for keytype in key_names:
             keyfile = KEY_FILE_TPL % (keytype)
             if os.path.exists(keyfile):
@@ -287,7 +285,9 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
             # TODO(harlowja): Is this guard needed?
             with util.SeLinuxGuard("/etc/ssh", recursive=True):
                 try:
-                    out, err = subp.subp(cmd, capture=True, env=lang_c)
+                    out, err = subp.subp(
+                        cmd, capture=True, update_env={"LANG": "C"}
+                    )
                     if not util.get_cfg_option_bool(
                         cfg, "ssh_quiet_keygen", False
                     ):

cloudinit/distros/bsd.py

@@ -130,7 +130,7 @@ def package_command(self, command, args=None, pkgs=None):
         cmd.extend(pkglist)
 
         # Allow the output of this to flow outwards (ie not be captured)
-        subp.subp(cmd, env=self._get_pkg_cmd_environ(), capture=False)
+        subp.subp(cmd, update_env=self._get_pkg_cmd_environ(), capture=False)
 
     def set_timezone(self, tz):
         distros.set_etc_timezone(tz=tz, tz_file=self._find_tz_file(tz))

cloudinit/distros/freebsd.py

@@ -201,9 +201,7 @@ def apply_locale(self, locale, out_fn=None):
     def _get_pkg_cmd_environ(self):
         """Return environment vars used in FreeBSD package_command
         operations"""
-        e = os.environ.copy()
-        e["ASSUME_ALWAYS_YES"] = "YES"
-        return e
+        return {"ASSUME_ALWAYS_YES": "YES"}
 
     def update_package_sources(self):
         self._runner.run(

cloudinit/distros/netbsd.py

@@ -146,14 +146,12 @@ def _get_pkg_cmd_environ(self):
         """Return env vars used in NetBSD package_command operations"""
         os_release = platform.release()
         os_arch = platform.machine()
-        e = os.environ.copy()
-        e[
-            "PKG_PATH"
-        ] = "http://cdn.netbsd.org/pub/pkgsrc/packages/NetBSD/%s/%s/All" % (
-            os_arch,
-            os_release,
-        )
-        return e
+        return {
+            "PKG_PATH": (
+                f"http://cdn.netbsd.org/pub/pkgsrc/packages/NetBSD"
+                f"/{os_arch}/{os_release}/All"
+            )
+        }
 
     def update_package_sources(self):
         pass

cloudinit/distros/openbsd.py

@@ -3,7 +3,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import logging
-import os
 
 import cloudinit.distros.netbsd
 from cloudinit import subp, util
@@ -58,5 +57,4 @@ def unlock_passwd(self, name):
 
     def _get_pkg_cmd_environ(self):
         """Return env vars used in OpenBSD package_command operations"""
-        e = os.environ.copy()
-        return e
+        return {}

cloudinit/distros/package_management/apt.py

@@ -91,8 +91,7 @@ def __init__(
         )
 
         self.apt_get_upgrade_subcommand = apt_get_upgrade_subcommand
-        self.environment = os.environ.copy()
-        self.environment["DEBIAN_FRONTEND"] = "noninteractive"
+        self.environment = {"DEBIAN_FRONTEND": "noninteractive"}
 
     @classmethod
     def from_config(cls, runner: helpers.Runners, cfg: Mapping) -> "Apt":
@@ -162,7 +161,7 @@ def run_package_command(self, command, args=None, pkgs=None):
             short_cmd=command,
             subp_kwargs={
                 "args": full_command,
-                "env": self.environment,
+                "update_env": self.environment,
                 "capture": False,
             },
         )

cloudinit/handlers/boot_hook.py

@@ -44,11 +44,13 @@ def handle_part(self, data, ctype, filename, payload, frequency):
 
         filepath = self._write_part(payload, filename)
         try:
-            env = os.environ.copy()
-            if self.instance_id is not None:
-                env["INSTANCE_ID"] = str(self.instance_id)
+            env = (
+                {"INSTANCE_ID": str(self.instance_id)}
+                if self.instance_id
+                else {}
+            )
             LOG.debug("Executing boothook")
-            subp.subp([filepath], env=env, capture=False)
+            subp.subp([filepath], update_env=env, capture=False)
         except subp.ProcessExecutionError:
             util.logexc(LOG, "Boothooks script %s execution error", filepath)
         except Exception:

cloudinit/subp.py

@@ -148,7 +148,6 @@ def subp(
     *,
     data=None,
     rcs=None,
-    env=None,
     capture=True,
     shell=False,
     logstring=False,
@@ -164,7 +163,6 @@ def subp(
         a list of allowed return codes.  If subprocess exits with a value not
         in this list, a ProcessExecutionError will be raised.  By default,
         data is returned as a string.  See 'decode' parameter.
-    :param env: a dictionary for the command's environment.
     :param capture:
         boolean indicating if output should be captured.  If True, then stderr
         and stdout will be returned.  If False, they will not be redirected.
@@ -178,7 +176,7 @@ def subp(
         These values are passed through to bytes().decode() as the 'errors'
         parameter.  There is no support for decoding to other than utf-8.
     :param update_env:
-        update the enviornment for this command with this dictionary.
+        update the environment for this command with this dictionary.
         this will not affect the current processes os.environ.
     :param cwd:
         change the working directory to cwd before executing the command.
@@ -195,10 +193,8 @@ def subp(
     if rcs is None:
         rcs = [0]
 
+    env = os.environ.copy()
     if update_env:
-        if env is None:
-            env = os.environ
-        env = env.copy()
         env.update(update_env)
 
     if not logstring:

tests/unittests/config/test_cc_ansible.py

@@ -1,6 +1,6 @@
+import os
 import re
 from copy import deepcopy
-from os import environ
 from textwrap import dedent
 from unittest import mock
 from unittest.mock import MagicMock
@@ -298,7 +298,6 @@ def test_required_keys(self, cfg, exception, mocker):
             M_PATH + "AnsiblePullDistro.is_installed",
             return_value=False,
         )
-        mocker.patch.dict(M_PATH + "os.environ", clear=True)
         if exception:
             with raises(exception):
                 cc_ansible.handle("", cfg, get_cloud(), None)
@@ -385,21 +384,20 @@ def test_ansible_pull(self, m_subp1, m_subp2, m_which, cfg, expected):
         """verify expected ansible invocation from userdata config"""
         pull_type = cfg["ansible"]["install_method"]
         distro = get_cloud().distro
-        with mock.patch.dict(M_PATH + "os.environ", clear=True):
-            ansible_pull = (
-                cc_ansible.AnsiblePullPip(distro, "ansible")
-                if pull_type == "pip"
-                else cc_ansible.AnsiblePullDistro(distro)
-            )
+        ansible_pull = (
+            cc_ansible.AnsiblePullPip(distro, "ansible")
+            if pull_type == "pip"
+            else cc_ansible.AnsiblePullDistro(distro)
+        )
         cc_ansible.run_ansible_pull(
             ansible_pull, deepcopy(cfg["ansible"]["pull"])
         )
 
         if pull_type != "pip":
             assert m_subp2.call_args[0][0] == expected
-            assert m_subp2.call_args[1]["env"].get("HOME") == environ.get(
+            assert m_subp2.call_args[1]["update_env"].get(
                 "HOME"
-            )
+            ) == os.environ.get("HOME", "/root")
 
     @mock.patch(M_PATH + "validate_config")
     def test_do_not_run(self, m_validate):
@@ -435,5 +433,5 @@ def test_ansible_env_var(self, m_which, m_subp):
         if isinstance(m_subp.call_args.kwargs, dict):
             assert (
                 "/etc/ansible/ansible.cfg"
-                == m_subp.call_args.kwargs["env"]["ansible_config"]
+                == m_subp.call_args.kwargs["update_env"]["ansible_config"]
             )