From 7680b81a3a37d50faf3f57422811990568f7dd58 Mon Sep 17 00:00:00 2001 From: Mitch Burton Date: Tue, 2 Jan 2024 16:22:18 -0800 Subject: [PATCH] apt.py: use CalledProcessError.stderr in exceptions instead of output/stdout (#115) * apt.py: use CalledProcessError.stderr in exceptions instead of output/stdout * bump LIBPATCH * use text=True in subprocess.run calls * integration tests for apt-get and apt-cache exceptions --- lib/charms/operator_libs_linux/v0/apt.py | 8 +++--- tests/integration/test_apt.py | 21 ++++++++++++++++ tests/unit/test_apt.py | 31 +++++++++++++++++++++++- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/lib/charms/operator_libs_linux/v0/apt.py b/lib/charms/operator_libs_linux/v0/apt.py index c3a2329e..1400df7f 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") @@ -250,10 +250,10 @@ 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.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/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) diff --git a/tests/unit/test_apt.py b/tests/unit/test_apt.py index 2cf3e9ca..c57a3dda 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") @@ -306,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) @@ -315,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"}, ) @@ -322,7 +342,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 +361,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", "") @@ -388,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) @@ -399,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) @@ -433,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( @@ -445,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) @@ -456,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)