From b9e6fa39dd21addd3f80db7359778b866bb97548 Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Thu, 14 Dec 2023 16:01:41 -0800 Subject: [PATCH 1/4] apt.py: use CalledProcessError.stderr in exceptions instead of output/stdout --- lib/charms/operator_libs_linux/v0/apt.py | 4 ++-- tests/unit/test_apt.py | 23 ++++++++++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/charms/operator_libs_linux/v0/apt.py b/lib/charms/operator_libs_linux/v0/apt.py index c3a2329e..2a2205af 100644 --- a/lib/charms/operator_libs_linux/v0/apt.py +++ b/lib/charms/operator_libs_linux/v0/apt.py @@ -253,7 +253,7 @@ def _apt( 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) + "Could not {} package(s) [{}]: {}".format(command, [*package_names], e.stderr) ) from None def _add(self) -> None: @@ -476,7 +476,7 @@ def from_apt_cache( ) except CalledProcessError as e: raise PackageError( - "Could not list packages in apt-cache: {}".format(e.output) + "Could not list packages in apt-cache: {}".format(e.stderr) ) from None pkg_groups = output.strip().split("\n\n") diff --git a/tests/unit/test_apt.py b/tests/unit/test_apt.py index 2cf3e9ca..99e48494 100644 --- a/tests/unit/test_apt.py +++ b/tests/unit/test_apt.py @@ -275,6 +275,24 @@ def test_can_load_from_apt_cache_multi_arch(self, mock_subprocess): self.assertEqual(tester.fullversion, "1:1.2.3-4.i386") self.assertEqual(str(tester.version), "1:1.2.3-4") + @patch("charms.operator_libs_linux.v0.apt.check_output") + def test_will_throw_apt_cache_errors(self, mock_subprocess): + mock_subprocess.side_effect = [ + "amd64", + subprocess.CalledProcessError( + returncode=100, + cmd=["apt-cache", "show", "mocktester"], + stderr="N: Unable to locate package mocktester", + ), + ] + + with self.assertRaises(apt.PackageError) as ctx: + apt.DebianPackage.from_apt_cache("mocktester", arch="i386") + + self.assertEqual("", ctx.exception.name) + self.assertIn("Could not list packages in apt-cache", ctx.exception.message) + self.assertIn("Unable to locate package", ctx.exception.message) + @patch("charms.operator_libs_linux.v0.apt.check_output") @patch("charms.operator_libs_linux.v0.apt.subprocess.run") @patch("os.environ.copy") @@ -322,7 +340,9 @@ def test_can_run_apt_commands( @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"] + returncode=1, + cmd=["apt-get", "-y", "install"], + stderr="E: Unable to locate package mocktester", ) mock_subprocess_output.side_effect = [ "amd64", @@ -339,6 +359,7 @@ def test_will_throw_apt_errors(self, mock_subprocess_call, mock_subprocess_outpu self.assertEqual("", ctx.exception.name) self.assertIn("Could not install package", ctx.exception.message) + self.assertIn("Unable to locate package", ctx.exception.message) def test_can_compare_versions(self): old_version = apt.Version("1.0.0", "") From e9f52f57434f92cded9204cdf79160e5220570ad Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Thu, 14 Dec 2023 16:05:26 -0800 Subject: [PATCH 2/4] 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 2a2205af..10538820 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 = 12 +LIBPATCH = 13 VALID_SOURCE_TYPES = ("deb", "deb-src") From 2137e5905778f0bdf791752df15a9b9cf6d51e26 Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Fri, 15 Dec 2023 13:32:44 -0800 Subject: [PATCH 3/4] use text=True in subprocess.run calls --- lib/charms/operator_libs_linux/v0/apt.py | 2 +- tests/unit/test_apt.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/charms/operator_libs_linux/v0/apt.py b/lib/charms/operator_libs_linux/v0/apt.py index 10538820..1400df7f 100644 --- a/lib/charms/operator_libs_linux/v0/apt.py +++ b/lib/charms/operator_libs_linux/v0/apt.py @@ -250,7 +250,7 @@ def _apt( try: env = os.environ.copy() env["DEBIAN_FRONTEND"] = "noninteractive" - subprocess.run(_cmd, capture_output=True, check=True, env=env) + subprocess.run(_cmd, capture_output=True, check=True, text=True, env=env) except CalledProcessError as e: raise PackageError( "Could not {} package(s) [{}]: {}".format(command, [*package_names], e.stderr) diff --git a/tests/unit/test_apt.py b/tests/unit/test_apt.py index 99e48494..c57a3dda 100644 --- a/tests/unit/test_apt.py +++ b/tests/unit/test_apt.py @@ -324,6 +324,7 @@ def test_can_run_apt_commands( ], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive", "PING": "PONG"}, ) self.assertEqual(pkg.state, apt.PackageState.Latest) @@ -333,6 +334,7 @@ def test_can_run_apt_commands( ["apt-get", "-y", "remove", "mocktester=1:1.2.3-4"], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive", "PING": "PONG"}, ) @@ -409,6 +411,7 @@ def test_can_run_bare_changes_on_single_package( ], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) self.assertEqual(foo.present, True) @@ -420,6 +423,7 @@ def test_can_run_bare_changes_on_single_package( ["apt-get", "-y", "remove", "zsh=5.8-3ubuntu1"], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) self.assertEqual(bar.present, False) @@ -454,6 +458,7 @@ def test_can_run_bare_changes_on_multiple_packages( ], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) mock_subprocess.assert_any_call( @@ -466,6 +471,7 @@ def test_can_run_bare_changes_on_multiple_packages( ], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) self.assertEqual(foo[0].present, True) @@ -477,12 +483,14 @@ def test_can_run_bare_changes_on_multiple_packages( ["apt-get", "-y", "remove", "vim=2:8.1.2269-1ubuntu5"], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) mock_subprocess.assert_any_call( ["apt-get", "-y", "remove", "zsh=5.8-3ubuntu1"], capture_output=True, check=True, + text=True, env={"DEBIAN_FRONTEND": "noninteractive"}, ) self.assertEqual(bar[0].present, False) From 06fe978ed321bdf43b26f5872a091951ea0a24cf Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Fri, 15 Dec 2023 13:47:36 -0800 Subject: [PATCH 4/4] integration tests for apt-get and apt-cache exceptions --- tests/integration/test_apt.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/integration/test_apt.py b/tests/integration/test_apt.py index ecbd4962..90c5147b 100644 --- a/tests/integration/test_apt.py +++ b/tests/integration/test_apt.py @@ -27,6 +27,20 @@ def test_install_package(): assert get_command_path("jq") == "/usr/bin/jq" +def test_install_package_error(): + try: + package = apt.DebianPackage( + "ceci-n'est-pas-un-paquet", + "1.0", + "", + "amd64", + apt.PackageState.Available, + ) + package.ensure(apt.PackageState.Present) + except apt.PackageError as e: + assert "Unable to locate package" in str(e) + + def test_remove_package(): # First ensure the package is present cfssl = apt.DebianPackage.from_apt_cache("golang-cfssl") @@ -77,3 +91,10 @@ def test_list_file_generation_external_repository(): apt.add_package("mongodb-org") assert get_command_path("mongod") == "/usr/bin/mongod" + + +def test_from_apt_cache_error(): + try: + apt.DebianPackage.from_apt_cache("ceci-n'est-pas-un-paquet") + except apt.PackageError as e: + assert "No packages found" in str(e)