From d669e2b60dbd31c6acf291b79328f7d32e1e69d5 Mon Sep 17 00:00:00 2001 From: Dan Jackson Date: Wed, 25 Nov 2020 21:59:32 -0800 Subject: [PATCH] Fix macports package present/active detection (#1308) * Fix typo in redhat_subscription testcase * Fix macports state=present matching against the wrong package name Previous implementation returned true if the desired package name occurred anywhere in the list of all installed packages. For example as a substring of another package name, or even as a substring of a variant name for a different package. Instead: - request macports only list installed packages matching the desired package name, instead of all installed packages. - Note `port` exits with 0 regardless of whether any packages match the requested name. - pass `-q` flag: "Do not print the header line. This is useful when parsing the output of port installed in scripts for further processing." - eliminate `use_unsafe_shell=True` by searching stdout contents natively in python instead of using `grep`. This has the added benefit of eliminating any potential misinterpretation of characters in the package name as regex special characters. If there are zero matching installed packages, `out` is empty. If there are one or more matches (due to multiple installed versions), the output format is: https://github.com/macports/macports-base/blob/ec8a2bc682c7afbde198aee23a1920557721b508/src/port/port.tcl#L3320-L3323 Notably, two leading spaces, the package name, a space, and then other information. According to blame via github, those lines haven't changed in 11 years. * Update macports state=active to eliminate use_unsafe_shell Similar to previous commit (for macports state=present): - pass `-q` flag: "Do not print the header line. This is useful when parsing the output of port installed in scripts for further processing." - search stdout contents natively in python instead of using `grep`. - added parentheses to search string to eliminate false positives if the package name or variants contain the word `active`. Still could fail if they contain `(active)`, but that's less likely If there are zero matching installed packages, `out` is empty. If there are one or more matches (due to multiple installed versions), the output format is: https://github.com/macports/macports-base/blob/ec8a2bc682c7afbde198aee23a1920557721b508/src/port/port.tcl#L3320-L3323 For "state=active", we're looking for a line that contains `(active)` in the output. * Basic test case of query_port for present and active * Attempt to fix lint errors in test * Different mock module creation, changed test cases indentation/spacing - picked the wrong mock code to cargo-cult. Thanks to felixfontein for this suggestion - 4 space indentation on continuation line. I thought I had that originally, but it looks like my editor sabotaged me with mixed tabs/spaces - Remove leading newline on multi-line test cases. I don't think it would make a difference, but I'd read up on how the python syntax works and want to more accurately represent macports output. fingers crossed this addresses the known build errors * Add changelog fragment * Update tests/unit/plugins/modules/packaging/os/test_macports.py Co-authored-by: Felix Fontein * Update changelogs/fragments/1307-macports-fix-status-check.yml Co-authored-by: Felix Fontein Co-authored-by: Felix Fontein --- .../1307-macports-fix-status-check.yml | 3 ++ plugins/modules/packaging/os/macports.py | 9 ++--- .../modules/packaging/os/test_macports.py | 34 +++++++++++++++++++ .../packaging/os/test_redhat_subscription.py | 6 ++-- 4 files changed, 45 insertions(+), 7 deletions(-) create mode 100644 changelogs/fragments/1307-macports-fix-status-check.yml create mode 100644 tests/unit/plugins/modules/packaging/os/test_macports.py diff --git a/changelogs/fragments/1307-macports-fix-status-check.yml b/changelogs/fragments/1307-macports-fix-status-check.yml new file mode 100644 index 0000000000..878e66ca39 --- /dev/null +++ b/changelogs/fragments/1307-macports-fix-status-check.yml @@ -0,0 +1,3 @@ +--- +bugfixes: + - macports - fix failure to install a package whose name is contained within an already installed package's name or variant (https://github.com/ansible-collections/community.general/issues/1307). diff --git a/plugins/modules/packaging/os/macports.py b/plugins/modules/packaging/os/macports.py index 5fc51e966d..a865a8f339 100644 --- a/plugins/modules/packaging/os/macports.py +++ b/plugins/modules/packaging/os/macports.py @@ -148,17 +148,18 @@ def query_port(module, port_path, name, state="present"): if state == "present": - rc, out, err = module.run_command("%s installed | grep -q ^.*%s" % (shlex_quote(port_path), shlex_quote(name)), use_unsafe_shell=True) - if rc == 0: + rc, out, err = module.run_command([port_path, "-q", "installed", name]) + + if rc == 0 and out.strip().startswith(name + " "): return True return False elif state == "active": - rc, out, err = module.run_command("%s installed %s | grep -q active" % (shlex_quote(port_path), shlex_quote(name)), use_unsafe_shell=True) + rc, out, err = module.run_command([port_path, "-q", "installed", name]) - if rc == 0: + if rc == 0 and "(active)" in out: return True return False diff --git a/tests/unit/plugins/modules/packaging/os/test_macports.py b/tests/unit/plugins/modules/packaging/os/test_macports.py new file mode 100644 index 0000000000..ef7c522f0f --- /dev/null +++ b/tests/unit/plugins/modules/packaging/os/test_macports.py @@ -0,0 +1,34 @@ +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +from ansible.module_utils import basic +from ansible_collections.community.general.plugins.modules.packaging.os import macports + +import pytest + +TESTED_MODULE = macports.__name__ + +QUERY_PORT_TEST_CASES = [ + pytest.param('', False, False, id='Not installed'), + pytest.param(' git @2.29.2_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28', True, False, id='Installed but not active'), + pytest.param(' git @2.29.2_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28 (active)', True, True, id='Installed and active'), + pytest.param(''' git @2.29.2_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28 + git @2.28.1_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28 +''', True, False, id='2 versions installed, neither active'), + pytest.param(''' git @2.29.2_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28 (active) + git @2.28.1_0+credential_osxkeychain+diff_highlight+doc+pcre+perl5_28 +''', True, True, id='2 versions installed, one active'), +] + + +@pytest.mark.parametrize("run_cmd_return_val, present_expected, active_expected", QUERY_PORT_TEST_CASES) +def test_macports_query_port(mocker, run_cmd_return_val, present_expected, active_expected): + module = mocker.Mock() + run_command = mocker.Mock() + run_command.return_value = (0, run_cmd_return_val, '') + module.run_command = run_command + + assert macports.query_port(module, 'port', 'git', state="present") == present_expected + assert macports.query_port(module, 'port', 'git', state="active") == active_expected diff --git a/tests/unit/plugins/modules/packaging/os/test_redhat_subscription.py b/tests/unit/plugins/modules/packaging/os/test_redhat_subscription.py index 2448ce67ed..ef6f28b812 100644 --- a/tests/unit/plugins/modules/packaging/os/test_redhat_subscription.py +++ b/tests/unit/plugins/modules/packaging/os/test_redhat_subscription.py @@ -18,7 +18,7 @@ TESTED_MODULE = redhat_subscription.__name__ @pytest.fixture def patch_redhat_subscription(mocker): """ - Function used for mocking some parts of redhat_subscribtion module + Function used for mocking some parts of redhat_subscription module """ mocker.patch('ansible_collections.community.general.plugins.modules.packaging.os.redhat_subscription.RegistrationBase.REDHAT_REPO') mocker.patch('ansible_collections.community.general.plugins.modules.packaging.os.redhat_subscription.isfile', return_value=False) @@ -817,7 +817,7 @@ TEST_CASES_IDS = [item[1]['id'] for item in TEST_CASES] @pytest.mark.parametrize('patch_ansible_module, testcase', TEST_CASES, ids=TEST_CASES_IDS, indirect=['patch_ansible_module']) @pytest.mark.usefixtures('patch_ansible_module') -def test_redhat_subscribtion(mocker, capfd, patch_redhat_subscription, testcase): +def test_redhat_subscription(mocker, capfd, patch_redhat_subscription, testcase): """ Run unit tests for test cases listen in TEST_CASES """ @@ -1173,7 +1173,7 @@ SYSPURPOSE_TEST_CASES_IDS = [item[1]['id'] for item in SYSPURPOSE_TEST_CASES] @pytest.mark.parametrize('patch_ansible_module, testcase', SYSPURPOSE_TEST_CASES, ids=SYSPURPOSE_TEST_CASES_IDS, indirect=['patch_ansible_module']) @pytest.mark.usefixtures('patch_ansible_module') -def test_redhat_subscribtion_syspurpose(mocker, capfd, patch_redhat_subscription, patch_ansible_module, testcase, tmpdir): +def test_redhat_subscription_syspurpose(mocker, capfd, patch_redhat_subscription, patch_ansible_module, testcase, tmpdir): """ Run unit tests for test cases listen in SYSPURPOSE_TEST_CASES (syspurpose specific cases) """