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

@@ -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")

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)

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("<charms.operator_libs_linux.v0.apt.PackageError>", 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,14 +334,17 @@ 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"},
         )
 
     @patch("charms.operator_libs_linux.v0.apt.check_output")
     @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("<charms.operator_libs_linux.v0.apt.PackageError>", 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)