From a9db4742fc44e74dea48ea7952bbe504acd0b410 Mon Sep 17 00:00:00 2001 From: Jean Raby Date: Tue, 1 Mar 2022 00:03:18 -0500 Subject: [PATCH] pacman: re-adding support for URL based pkgs (#4286) * pacman: re-adding support for URL based pkgs * Update plugins/modules/packaging/os/pacman.py Co-authored-by: Felix Fontein * Update plugins/modules/packaging/os/pacman.py Co-authored-by: Felix Fontein * cmd=cmd in every call to self.fail() * pacman: integration test for mixed pkg sources * Add more tests + fix minor bug with URL packages Version checking for URL packages is left to pacman, so add a check after the dry run to see if it would actually install anything. * remove double templating Co-authored-by: Felix Fontein --- changelogs/fragments/4286-pacman-url-pkgs.yml | 3 + plugins/modules/packaging/os/pacman.py | 120 ++++++++++++----- .../integration/targets/pacman/tasks/main.yml | 1 + .../targets/pacman/tasks/package_urls.yml | 125 ++++++++++++++++++ .../modules/packaging/os/test_pacman.py | 97 ++++++++++++-- 5 files changed, 300 insertions(+), 46 deletions(-) create mode 100644 changelogs/fragments/4286-pacman-url-pkgs.yml create mode 100644 tests/integration/targets/pacman/tasks/package_urls.yml diff --git a/changelogs/fragments/4286-pacman-url-pkgs.yml b/changelogs/fragments/4286-pacman-url-pkgs.yml new file mode 100644 index 0000000000..12af9820c9 --- /dev/null +++ b/changelogs/fragments/4286-pacman-url-pkgs.yml @@ -0,0 +1,3 @@ +bugfixes: + - pacman - fix URL based package installation + (https://github.com/ansible-collections/community.general/pull/4286, https://github.com/ansible-collections/community.general/issues/4285). diff --git a/plugins/modules/packaging/os/pacman.py b/plugins/modules/packaging/os/pacman.py index 4eec2499d1..116ed48e6d 100644 --- a/plugins/modules/packaging/os/pacman.py +++ b/plugins/modules/packaging/os/pacman.py @@ -190,7 +190,22 @@ from ansible.module_utils.basic import AnsibleModule from collections import defaultdict, namedtuple -Package = namedtuple("Package", ["name", "source"]) +class Package(object): + def __init__(self, name, source, source_is_URL=False): + self.name = name + self.source = source + self.source_is_URL = source_is_URL + + def __eq__(self, o): + return self.name == o.name and self.source == o.source and self.source_is_URL == o.source_is_URL + + def __lt__(self, o): + return self.name < o.name + + def __repr__(self): + return 'Package("%s", "%s", %s)' % (self.name, self.source, self.source_is_URL) + + VersionTuple = namedtuple("VersionTuple", ["current", "latest"]) @@ -273,7 +288,13 @@ class Pacman(object): def install_packages(self, pkgs): pkgs_to_install = [] + pkgs_to_install_from_url = [] for p in pkgs: + if p.source_is_URL: + # URL packages bypass the latest / upgradable_pkgs test + # They go through the dry-run to let pacman decide if they will be installed + pkgs_to_install_from_url.append(p) + continue if ( p.name not in self.inventory["installed_pkgs"] or self.target_state == "latest" @@ -281,14 +302,12 @@ class Pacman(object): ): pkgs_to_install.append(p) - if len(pkgs_to_install) == 0: + if len(pkgs_to_install) == 0 and len(pkgs_to_install_from_url) == 0: self.add_exit_infos("package(s) already installed") return - self.changed = True cmd_base = [ self.pacman_path, - "--sync", "--noconfirm", "--noprogressbar", "--needed", @@ -296,45 +315,78 @@ class Pacman(object): if self.m.params["extra_args"]: cmd_base.extend(self.m.params["extra_args"]) - # Dry run first to gather what will be done - cmd = cmd_base + ["--print-format", "%n %v"] + [p.source for p in pkgs_to_install] - rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) - if rc != 0: - self.fail("Failed to list package(s) to install", stdout=stdout, stderr=stderr) + def _build_install_diff(pacman_verb, pkglist): + # Dry run to build the installation diff + + cmd = cmd_base + [pacman_verb, "--print-format", "%n %v"] + [p.source for p in pkglist] + rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) + if rc != 0: + self.fail("Failed to list package(s) to install", cmd=cmd, stdout=stdout, stderr=stderr) + + name_ver = [l.strip() for l in stdout.splitlines()] + before = [] + after = [] + to_be_installed = [] + for p in name_ver: + # With Pacman v6.0.1 - libalpm v13.0.1, --upgrade outputs "loading packages..." on stdout. strip that. + # When installing from URLs, pacman can also output a 'nothing to do' message. strip that too. + if "loading packages" in p or 'there is nothing to do' in p: + continue + name, version = p.split() + if name in self.inventory["installed_pkgs"]: + before.append("%s-%s" % (name, self.inventory["installed_pkgs"][name])) + after.append("%s-%s" % (name, version)) + to_be_installed.append(name) + + return (to_be_installed, before, after) - name_ver = [l.strip() for l in stdout.splitlines()] before = [] after = [] installed_pkgs = [] - self.exit_params["packages"] = [] - for p in name_ver: - name, version = p.split() - if name in self.inventory["installed_pkgs"]: - before.append("%s-%s" % (name, self.inventory["installed_pkgs"][name])) - after.append("%s-%s" % (name, version)) - installed_pkgs.append(name) + + if pkgs_to_install: + p, b, a = _build_install_diff("--sync", pkgs_to_install) + installed_pkgs.extend(p) + before.extend(b) + after.extend(a) + if pkgs_to_install_from_url: + p, b, a = _build_install_diff("--upgrade", pkgs_to_install_from_url) + installed_pkgs.extend(p) + before.extend(b) + after.extend(a) + + if len(installed_pkgs) == 0: + # This can happen with URL packages if pacman decides there's nothing to do + self.add_exit_infos("package(s) already installed") + return + + self.changed = True self.exit_params["diff"] = { - "before": "\n".join(before) + "\n" if before else "", - "after": "\n".join(after) + "\n" if after else "", + "before": "\n".join(sorted(before)) + "\n" if before else "", + "after": "\n".join(sorted(after)) + "\n" if after else "", } if self.m.check_mode: self.add_exit_infos("Would have installed %d packages" % len(installed_pkgs)) - self.exit_params["packages"] = installed_pkgs + self.exit_params["packages"] = sorted(installed_pkgs) return # actually do it - cmd = cmd_base + [p.source for p in pkgs_to_install] + def _install_packages_for_real(pacman_verb, pkglist): + cmd = cmd_base + [pacman_verb] + [p.source for p in pkglist] + rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) + if rc != 0: + self.fail("Failed to install package(s)", cmd=cmd, stdout=stdout, stderr=stderr) + self.add_exit_infos(stdout=stdout, stderr=stderr) - rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) - if rc != 0: - self.fail("Failed to install package(s)", stdout=stdout, stderr=stderr) + if pkgs_to_install: + _install_packages_for_real("--sync", pkgs_to_install) + if pkgs_to_install_from_url: + _install_packages_for_real("--upgrade", pkgs_to_install_from_url) self.exit_params["packages"] = installed_pkgs - self.add_exit_infos( - "Installed %d package(s)" % len(installed_pkgs), stdout=stdout, stderr=stderr - ) + self.add_exit_infos("Installed %d package(s)" % len(installed_pkgs)) def remove_packages(self, pkgs): force_args = ["--nodeps", "--nodeps"] if self.m.params["force"] else [] @@ -362,7 +414,7 @@ class Pacman(object): rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) if rc != 0: - self.fail("failed to list package(s) to remove", stdout=stdout, stderr=stderr) + self.fail("failed to list package(s) to remove", cmd=cmd, stdout=stdout, stderr=stderr) removed_pkgs = stdout.split() self.exit_params["packages"] = removed_pkgs @@ -381,7 +433,7 @@ class Pacman(object): rc, stdout, stderr = self.m.run_command(cmd, check_rc=False) if rc != 0: - self.fail("failed to remove package(s)", stdout=stdout, stderr=stderr) + self.fail("failed to remove package(s)", cmd=cmd, stdout=stdout, stderr=stderr) self.exit_params["packages"] = removed_pkgs self.add_exit_infos("Removed %d package(s)" % len(removed_pkgs), stdout=stdout, stderr=stderr) @@ -420,7 +472,7 @@ class Pacman(object): if rc == 0: self.add_exit_infos("System upgraded", stdout=stdout, stderr=stderr) else: - self.fail("Could not upgrade", stdout=stdout, stderr=stderr) + self.fail("Could not upgrade", cmd=cmd, stdout=stdout, stderr=stderr) def update_package_db(self): """runs pacman --sync --refresh""" @@ -446,7 +498,7 @@ class Pacman(object): if rc == 0: self.add_exit_infos("Updated package db", stdout=stdout, stderr=stderr) else: - self.fail("could not update package db", stdout=stdout, stderr=stderr) + self.fail("could not update package db", cmd=cmd, stdout=stdout, stderr=stderr) def package_list(self): """Takes the input package list and resolves packages groups to their package list using the inventory, @@ -459,6 +511,7 @@ class Pacman(object): if not pkg: continue + is_URL = False if pkg in self.inventory["available_groups"]: # Expand group members for group_member in self.inventory["available_groups"][pkg]: @@ -488,8 +541,11 @@ class Pacman(object): stderr=stderr, rc=rc, ) + # With Pacman v6.0.1 - libalpm v13.0.1, --upgrade outputs "loading packages..." on stdout. strip that + stdout = stdout.replace("loading packages...\n", "") + is_URL = True pkg_name = stdout.strip() - pkg_list.append(Package(name=pkg_name, source=pkg)) + pkg_list.append(Package(name=pkg_name, source=pkg, source_is_URL=is_URL)) return pkg_list diff --git a/tests/integration/targets/pacman/tasks/main.yml b/tests/integration/targets/pacman/tasks/main.yml index 02b4035317..f19262811c 100644 --- a/tests/integration/targets/pacman/tasks/main.yml +++ b/tests/integration/targets/pacman/tasks/main.yml @@ -8,3 +8,4 @@ block: # Add more tests here by including more task files: - include: 'basic.yml' + - include: 'package_urls.yml' diff --git a/tests/integration/targets/pacman/tasks/package_urls.yml b/tests/integration/targets/pacman/tasks/package_urls.yml new file mode 100644 index 0000000000..4bb897b404 --- /dev/null +++ b/tests/integration/targets/pacman/tasks/package_urls.yml @@ -0,0 +1,125 @@ +--- +- vars: + reg_pkg: ed + url_pkg: lemon + file_pkg: hdparm + file_pkg_path: /tmp/pkg.zst + extra_pkg: core/sdparm + extra_pkg_outfmt: sdparm + block: + - name: Make sure that test packages are not installed + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg}}' + - '{{file_pkg}}' + - '{{extra_pkg}}' + state: absent + + - name: Get URL for {{url_pkg}} + command: + cmd: pacman --sync --print-format "%l" {{url_pkg}} + register: url_pkg_url + + - name: Get URL for {{file_pkg}} + command: + cmd: pacman --sync --print-format "%l" {{file_pkg}} + register: file_pkg_url + - name: Download {{file_pkg}} pkg + get_url: + url: '{{file_pkg_url.stdout}}' + dest: '{{file_pkg_path}}' + + - name: Install packages from mixed sources (check mode) + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + check_mode: True + register: install_1 + + - name: Install packages from mixed sources + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + register: install_2 + + - name: Install packages from mixed sources - (idempotency) + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + register: install_3 + + - name: Install packages with their regular names (idempotency) + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg}}' + - '{{file_pkg}}' + register: install_4 + + - name: Install new package with already installed packages from mixed sources + pacman: + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + - '{{extra_pkg}}' + register: install_5 + + - name: Uninstall packages - mixed sources (check mode) + pacman: + state: absent + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + check_mode: True + register: uninstall_1 + + - name: Uninstall packages - mixed sources + pacman: + state: absent + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + register: uninstall_2 + + - name: Uninstall packages - mixed sources (idempotency) + pacman: + state: absent + name: + - '{{reg_pkg}}' + - '{{url_pkg_url.stdout}}' + - '{{file_pkg_path}}' + register: uninstall_3 + + - assert: + that: + - install_1 is changed + - install_1.msg == 'Would have installed 3 packages' + - install_1.packages|sort() == [reg_pkg, url_pkg, file_pkg]|sort() + - install_2 is changed + - install_2.msg == 'Installed 3 package(s)' + - install_1.packages|sort() == [reg_pkg, url_pkg, file_pkg]|sort() + - install_3 is not changed + - install_3.msg == 'package(s) already installed' + - install_4 is not changed + - install_4.msg == 'package(s) already installed' + - install_5 is changed + - install_5.msg == 'Installed 1 package(s)' + - install_5.packages == [extra_pkg_outfmt] + - uninstall_1 is changed + - uninstall_1.msg == 'Would have removed 3 packages' + - uninstall_1.packages | length() == 3 # pkgs have versions here + - uninstall_2 is changed + - uninstall_2.msg == 'Removed 3 package(s)' + - uninstall_2.packages | length() == 3 + - uninstall_3 is not changed + - uninstall_3.msg == 'package(s) already absent' diff --git a/tests/unit/plugins/modules/packaging/os/test_pacman.py b/tests/unit/plugins/modules/packaging/os/test_pacman.py index b0a92fb274..f0097bc619 100644 --- a/tests/unit/plugins/modules/packaging/os/test_pacman.py +++ b/tests/unit/plugins/modules/packaging/os/test_pacman.py @@ -475,7 +475,13 @@ class TestPacman: # catch all -> call to pacman to resolve (--sync and --upgrade) "present", ["somepackage-12.3-x86_64.pkg.tar.zst"], - [Package(name="somepackage", source="somepackage-12.3-x86_64.pkg.tar.zst")], + [ + Package( + name="somepackage", + source="somepackage-12.3-x86_64.pkg.tar.zst", + source_is_URL=True, + ) + ], { "calls": [ mock.call( @@ -582,12 +588,13 @@ class TestPacman: self.mock_run_command.call_count == 0 @pytest.mark.parametrize( - "module_args, expected_packages, run_command_data, raises", + "module_args, expected_packages, package_list_out, run_command_data, raises", [ ( # remove pkg: Check mode -- call to print format but that's it {"_ansible_check_mode": True, "name": ["grep"], "state": "absent"}, ["grep-version"], + [Package("grep", "grep")], { "calls": [ mock.call( @@ -612,6 +619,7 @@ class TestPacman: # remove pkg for real now -- with 2 packages {"name": ["grep", "gawk"], "state": "absent"}, ["grep-version", "gawk-anotherversion"], + [Package("grep", "grep"), Package("gawk", "gawk")], { "calls": [ mock.call( @@ -650,6 +658,7 @@ class TestPacman: "extra_args": "--some --extra arg", }, ["grep-version"], + [Package("grep", "grep")], { "calls": [ mock.call( @@ -698,6 +707,7 @@ class TestPacman: # remove pkg -- Failure to list {"name": ["grep"], "state": "absent"}, ["grep-3.7-1"], + [Package("grep", "grep")], { "calls": [ mock.call( @@ -724,6 +734,7 @@ class TestPacman: # remove pkg -- Failure to remove {"name": ["grep"], "state": "absent"}, ["grep-3.7-1"], + [Package("grep", "grep")], { "calls": [ mock.call( @@ -756,16 +767,17 @@ class TestPacman: # install pkg: Check mode {"_ansible_check_mode": True, "name": ["sudo"], "state": "present"}, ["sudo"], + [Package("sudo", "sudo")], { "calls": [ mock.call( mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--sync", "--print-format", "%n %v", "sudo", @@ -778,19 +790,37 @@ class TestPacman: AnsibleExitJson, ), ( - # install 2 pkgs, one already present - {"name": ["sudo", "grep"], "state": "present"}, - ["sudo"], + # Install pkgs: one regular, one already installed, one file URL and one https URL + { + "name": [ + "sudo", + "grep", + "./somepackage-12.3-x86_64.pkg.tar.zst", + "http://example.com/otherpkg-1.2-x86_64.pkg.tar.zst", + ], + "state": "present", + }, + ["sudo", "somepackage", "otherpkg"], + [ + Package("sudo", "sudo"), + Package("grep", "grep"), + Package("somepackage", "./somepackage-12.3-x86_64.pkg.tar.zst", source_is_URL=True), + Package( + "otherpkg", + "http://example.com/otherpkg-1.2-x86_64.pkg.tar.zst", + source_is_URL=True, + ), + ], { "calls": [ mock.call( mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--sync", "--print-format", "%n %v", "sudo", @@ -801,16 +831,49 @@ class TestPacman: mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--upgrade", + "--print-format", + "%n %v", + "./somepackage-12.3-x86_64.pkg.tar.zst", + "http://example.com/otherpkg-1.2-x86_64.pkg.tar.zst", + ], + check_rc=False, + ), + mock.call( + mock.ANY, + [ + "pacman", + "--noconfirm", + "--noprogressbar", + "--needed", + "--sync", "sudo", ], check_rc=False, ), + mock.call( + mock.ANY, + [ + "pacman", + "--noconfirm", + "--noprogressbar", + "--needed", + "--upgrade", + "./somepackage-12.3-x86_64.pkg.tar.zst", + "http://example.com/otherpkg-1.2-x86_64.pkg.tar.zst", + ], + check_rc=False, + ), + ], + "side_effect": [ + (0, "sudo version", ""), + (0, "somepackage 12.3\notherpkg 1.2", ""), + (0, "", ""), + (0, "", ""), ], - "side_effect": [(0, "sudo version", ""), (0, "", "")], }, AnsibleExitJson, ), @@ -818,19 +881,20 @@ class TestPacman: # install pkg, extra_args {"name": ["sudo"], "state": "present", "extra_args": "--some --thing else"}, ["sudo"], + [Package("sudo", "sudo")], { "calls": [ mock.call( mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", "--some", "--thing", "else", + "--sync", "--print-format", "%n %v", "sudo", @@ -841,13 +905,13 @@ class TestPacman: mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", "--some", "--thing", "else", + "--sync", "sudo", ], check_rc=False, @@ -861,16 +925,17 @@ class TestPacman: # latest pkg: Check mode {"_ansible_check_mode": True, "name": ["sqlite"], "state": "latest"}, ["sqlite"], + [Package("sqlite", "sqlite")], { "calls": [ mock.call( mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--sync", "--print-format", "%n %v", "sqlite", @@ -886,16 +951,17 @@ class TestPacman: # latest pkg -- one already latest {"name": ["sqlite", "grep"], "state": "latest"}, ["sqlite"], + [Package("sqlite", "sqlite")], { "calls": [ mock.call( mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--sync", "--print-format", "%n %v", "sqlite", @@ -906,10 +972,10 @@ class TestPacman: mock.ANY, [ "pacman", - "--sync", "--noconfirm", "--noprogressbar", "--needed", + "--sync", "sqlite", ], check_rc=False, @@ -924,13 +990,16 @@ class TestPacman: def test_op_packages( self, mock_valid_inventory, + mock_package_list, module_args, expected_packages, + package_list_out, run_command_data, raises, ): set_module_args(module_args) self.mock_run_command.side_effect = run_command_data["side_effect"] + mock_package_list.return_value = package_list_out P = pacman.Pacman(pacman.setup_module()) with pytest.raises(raises) as e: