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: ec8a2bc682/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: ec8a2bc682/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 <felix@fontein.de>

* Update changelogs/fragments/1307-macports-fix-status-check.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>

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).

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)
+        rc, out, err = module.run_command([port_path, "-q", "installed", name])
-        if rc == 0:
+
+        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

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

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