Skip to content

Commit

Permalink
apt.py: use subprocess.run with check=True instead of check_call when…
Browse files Browse the repository at this point in the history 
… capturing stdout and stderr (#114)

* apt.py: use subprocess.run with check=True instead of check_call when
capturing stdout and stderr

* bump LIBPATCH
  • Loading branch information
@Perfect5th
Perfect5th authored Dec 14, 2023
1 parent 20880b1 commit 405948a
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 29 deletions.
8 changes: 4 additions & 4 deletions lib/charms/operator_libs_linux/v0/apt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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")
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down
50 changes: 25 additions & 25 deletions tests/unit/test_apt.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"]
Expand Down Expand Up @@ -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
Expand All @@ -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)

Expand All @@ -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
Expand All @@ -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(
[
Expand All @@ -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)
Expand All @@ -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 = [
Expand All @@ -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 = [
Expand All @@ -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("<charms.operator_libs_linux.v0.apt.PackageError>", 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]

Expand Down

0 comments on commit 405948a

Please sign in to comment.