From a1b80b35514b2b1358745f04635596a81eaf7810 Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Thu, 14 Dec 2023 11:24:41 -0800 Subject: [PATCH 1/2] apt.py: use subprocess.run with check=True instead of check_call when capturing stdout and stderr --- lib/charms/operator_libs_linux/v0/apt.py | 6 +-- tests/unit/test_apt.py | 50 ++++++++++++------------ 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/lib/charms/operator_libs_linux/v0/apt.py b/lib/charms/operator_libs_linux/v0/apt.py index 2fc1dc75..5722d6ea 100644 --- a/lib/charms/operator_libs_linux/v0/apt.py +++ b/lib/charms/operator_libs_linux/v0/apt.py @@ -108,7 +108,7 @@ import subprocess from collections.abc import Mapping from enum import Enum -from subprocess import PIPE, CalledProcessError, check_call, check_output +from subprocess import PIPE, CalledProcessError, check_output from typing import Iterable, List, Optional, Tuple, Union from urllib.parse import urlparse @@ -250,7 +250,7 @@ def _apt( try: env = os.environ.copy() env["DEBIAN_FRONTEND"] = "noninteractive" - check_call(_cmd, env=env, stderr=PIPE, stdout=PIPE) + subprocess.run(_cmd, capture_output=True, check=True, env=env) except CalledProcessError as e: raise PackageError( "Could not {} package(s) [{}]: {}".format(command, [*package_names], e.output) @@ -837,7 +837,7 @@ def remove_package( def update() -> None: """Update the apt cache via `apt-get update`.""" - check_call(["apt-get", "update"], stderr=PIPE, stdout=PIPE) + subprocess.run(["apt-get", "update"], capture_output=True, check=True) def import_key(key: str) -> str: diff --git a/tests/unit/test_apt.py b/tests/unit/test_apt.py index d593a893..2cf3e9ca 100644 --- a/tests/unit/test_apt.py +++ b/tests/unit/test_apt.py @@ -276,7 +276,7 @@ def test_can_load_from_apt_cache_multi_arch(self, mock_subprocess): self.assertEqual(str(tester.version), "1:1.2.3-4") @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") @patch("os.environ.copy") def test_can_run_apt_commands( self, mock_environ, mock_subprocess_call, mock_subprocess_output @@ -304,22 +304,22 @@ def test_can_run_apt_commands( "install", "mocktester=1:1.2.3-4", ], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive", "PING": "PONG"}, - stderr=-1, - stdout=-1, ) self.assertEqual(pkg.state, apt.PackageState.Latest) pkg.state = apt.PackageState.Absent mock_subprocess_call.assert_called_with( ["apt-get", "-y", "remove", "mocktester=1:1.2.3-4"], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive", "PING": "PONG"}, - stdout=-1, - stderr=-1, ) @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") def test_will_throw_apt_errors(self, mock_subprocess_call, mock_subprocess_output): mock_subprocess_call.side_effect = subprocess.CalledProcessError( returncode=1, cmd=["apt-get", "-y", "install"] @@ -363,7 +363,7 @@ def test_can_parse_epoch_and_version(self): class TestAptBareMethods(unittest.TestCase): @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") @patch("os.environ.copy") def test_can_run_bare_changes_on_single_package( self, mock_environ, mock_subprocess, mock_subprocess_output @@ -386,9 +386,9 @@ def test_can_run_bare_changes_on_single_package( "install", "aisleriot=1:3.22.9-1", ], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) self.assertEqual(foo.present, True) @@ -397,14 +397,14 @@ def test_can_run_bare_changes_on_single_package( bar.ensure(apt.PackageState.Absent) mock_subprocess.assert_called_with( ["apt-get", "-y", "remove", "zsh=5.8-3ubuntu1"], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) self.assertEqual(bar.present, False) @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") @patch("os.environ.copy") def test_can_run_bare_changes_on_multiple_packages( self, mock_environ, mock_subprocess, mock_subprocess_output @@ -431,9 +431,9 @@ def test_can_run_bare_changes_on_multiple_packages( "install", "aisleriot=1:3.22.9-1", ], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) mock_subprocess.assert_any_call( [ @@ -443,9 +443,9 @@ def test_can_run_bare_changes_on_multiple_packages( "install", "mocktester=1:1.2.3-4", ], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) self.assertEqual(foo[0].present, True) self.assertEqual(foo[1].present, True) @@ -454,21 +454,21 @@ def test_can_run_bare_changes_on_multiple_packages( bar = apt.remove_package(["vim", "zsh"]) mock_subprocess.assert_any_call( ["apt-get", "-y", "remove", "vim=2:8.1.2269-1ubuntu5"], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) mock_subprocess.assert_any_call( ["apt-get", "-y", "remove", "zsh=5.8-3ubuntu1"], + capture_output=True, + check=True, env={"DEBIAN_FRONTEND": "noninteractive"}, - stderr=-1, - stdout=-1, ) self.assertEqual(bar[0].present, False) self.assertEqual(bar[1].present, False) @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") def test_refreshes_apt_cache_if_not_found(self, mock_subprocess, mock_subprocess_output): mock_subprocess.return_value = 0 mock_subprocess_output.side_effect = [ @@ -482,12 +482,12 @@ def test_refreshes_apt_cache_if_not_found(self, mock_subprocess, mock_subprocess apt_cache_aisleriot, ] pkg = apt.add_package("aisleriot") - mock_subprocess.assert_any_call(["apt-get", "update"], stderr=-1, stdout=-1) + mock_subprocess.assert_any_call(["apt-get", "update"], capture_output=True, check=True) self.assertEqual(pkg.name, "aisleriot") self.assertEqual(pkg.present, True) @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") def test_raises_package_not_found_error(self, mock_subprocess, mock_subprocess_output): mock_subprocess.return_value = 0 mock_subprocess_output.side_effect = [ @@ -498,12 +498,12 @@ def test_raises_package_not_found_error(self, mock_subprocess, mock_subprocess_o ] * 2 # Double up for the retry after update with self.assertRaises(apt.PackageError) as ctx: apt.add_package("nothere") - mock_subprocess.assert_any_call(["apt-get", "update"], stderr=-1, stdout=-1) + mock_subprocess.assert_any_call(["apt-get", "update"], capture_output=True, check=True) self.assertEqual("", ctx.exception.name) self.assertIn("Failed to install packages: nothere", ctx.exception.message) @patch("charms.operator_libs_linux.v0.apt.check_output") - @patch("charms.operator_libs_linux.v0.apt.check_call") + @patch("charms.operator_libs_linux.v0.apt.subprocess.run") def test_remove_package_not_installed(self, mock_subprocess, mock_subprocess_output): mock_subprocess_output.side_effect = ["amd64", dpkg_output_not_installed] From da79071c3f9c96ea43a561fc57dad0bc8e70765f Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Thu, 14 Dec 2023 11:54:02 -0800 Subject: [PATCH 2/2] bump LIBPATCH --- lib/charms/operator_libs_linux/v0/apt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/charms/operator_libs_linux/v0/apt.py b/lib/charms/operator_libs_linux/v0/apt.py index 5722d6ea..c3a2329e 100644 --- a/lib/charms/operator_libs_linux/v0/apt.py +++ b/lib/charms/operator_libs_linux/v0/apt.py @@ -122,7 +122,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 11 +LIBPATCH = 12 VALID_SOURCE_TYPES = ("deb", "deb-src")