Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1b80b3
apt.py: use subprocess.run with check=True instead of check_call when
Perfect5th Dec 14, 2023
da79071
bump LIBPATCH
Perfect5th Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/charms/operator_libs_linux/v0/apt.py
View file
Open in desktop
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 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
Open in desktop
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