ansible-test - Expand unwanted pylint check (#86416)

Also remove redundant tests from the validate-modules sanity test.

changelogs/fragments/ansible-test-sanity-unwanted.yml

@@ -0,0 +1,5 @@
+minor_changes:
+  - ansible-test - Expand functions covered by the ``unwanted`` rule for the ``pylint`` sanity test.
+    It now includes various ``os.*`` and ``subprocess.*`` subprocess functions in Ansible modules and module_utils.
+  - ansible-test - Remove ``use-run-command-not-popen`` and ``use-run-command-not-os-call`` error codes from the
+    ``validate-modules`` sanity test. These scenarios are now covered by the ``pylint`` sanity test.

test/integration/targets/ansible-test-sanity-pylint/ansible_collections/ns/col/plugins/module_utils/unwanted.py

@@ -0,0 +1,31 @@
+from __future__ import annotations
+
+import os
+import subprocess
+import sys
+
+
+def main() -> None:
+    os.popen('echo')
+    os.posix_spawn('echo', ['echo'], {})
+    os.posix_spawnp('echo', ['echo'], {})
+    os.spawnl(os.P_WAIT, 'echo', 'echo')
+    os.spawnle(os.P_WAIT, 'echo', 'echo', {})
+    os.spawnlp(os.P_WAIT, 'echo', 'echo')
+    os.spawnlpe(os.P_WAIT, 'echo', 'echo', {})
+    os.spawnv(os.P_WAIT, 'echo', ['echo'])
+    os.spawnve(os.P_WAIT, 'echo', ['echo'], {})
+    os.spawnvp(os.P_WAIT, 'echo', ['echo'])
+    os.spawnvpe(os.P_WAIT, 'echo', ['echo'], {})
+    os.system('echo')
+
+    subprocess.Popen('echo')
+    subprocess.call('echo')
+    subprocess.check_call('echo')
+    subprocess.check_output('echo')
+    subprocess.getoutput('echo')
+    subprocess.getstatusoutput('echo')
+    subprocess.run('echo', check=True)
+
+    print()
+    sys.exit(0)

test/integration/targets/ansible-test-sanity-pylint/expected.txt

@@ -39,3 +39,24 @@ plugins/module_utils/deprecated_utils.py:28:4: ansible-invalid-deprecated-date:
 plugins/module_utils/deprecated_utils.py:29:4: ansible-deprecated-both-version-and-date: Both version and date found in call to 'ansible.module_utils.common.warnings.deprecate'
 plugins/module_utils/deprecated_utils.py:30:4: removal-version-must-be-major: Removal version '3.1.0' must be a major release, not a minor or patch release, see https://semver.org/
 plugins/module_utils/deprecated_utils.py:34:4: ansible-deprecated-both-collection-name-and-deprecator: Both collection_name and deprecator found in call to 'ansible.module_utils.common.warnings.deprecate'
+plugins/module_utils/unwanted.py:9:4: ansible-bad-function: Call run_command instead of os.popen
+plugins/module_utils/unwanted.py:10:4: ansible-bad-function: Call run_command instead of os.posix_spawn
+plugins/module_utils/unwanted.py:11:4: ansible-bad-function: Call run_command instead of os.posix_spawnp
+plugins/module_utils/unwanted.py:12:4: ansible-bad-function: Call run_command instead of os.spawnl
+plugins/module_utils/unwanted.py:13:4: ansible-bad-function: Call run_command instead of os.spawnle
+plugins/module_utils/unwanted.py:14:4: ansible-bad-function: Call run_command instead of os.spawnlp
+plugins/module_utils/unwanted.py:15:4: ansible-bad-function: Call run_command instead of os.spawnlpe
+plugins/module_utils/unwanted.py:16:4: ansible-bad-function: Call run_command instead of os.spawnv
+plugins/module_utils/unwanted.py:17:4: ansible-bad-function: Call run_command instead of os.spawnve
+plugins/module_utils/unwanted.py:18:4: ansible-bad-function: Call run_command instead of os.spawnvp
+plugins/module_utils/unwanted.py:19:4: ansible-bad-function: Call run_command instead of os.spawnvpe
+plugins/module_utils/unwanted.py:20:4: ansible-bad-function: Call run_command instead of os.system
+plugins/module_utils/unwanted.py:22:4: ansible-bad-function: Call run_command instead of subprocess.Popen
+plugins/module_utils/unwanted.py:23:4: ansible-bad-function: Call run_command instead of subprocess.call
+plugins/module_utils/unwanted.py:24:4: ansible-bad-function: Call run_command instead of subprocess.check_call
+plugins/module_utils/unwanted.py:25:4: ansible-bad-function: Call run_command instead of subprocess.check_output
+plugins/module_utils/unwanted.py:26:4: ansible-bad-function: Call run_command instead of subprocess.getoutput
+plugins/module_utils/unwanted.py:27:4: ansible-bad-function: Call run_command instead of subprocess.getstatusoutput
+plugins/module_utils/unwanted.py:28:4: ansible-bad-function: Call run_command instead of subprocess.run
+plugins/module_utils/unwanted.py:30:4: ansible-bad-function: Call module.log or module.debug instead of builtins.print
+plugins/module_utils/unwanted.py:31:4: ansible-bad-function: Call exit_json or fail_json instead of sys.exit

test/lib/ansible_test/_util/controller/sanity/pylint/plugins/unwanted.py

@@ -113,9 +113,7 @@ class AnsibleUnwantedChecker(BaseChecker):
         # see https://docs.python.org/3/library/tempfile.html#tempfile.mktemp
         'tempfile.mktemp': UnwantedEntry('tempfile.mkstemp'),
 
-        # os.chmod resolves as posix.chmod
-        'posix.chmod': UnwantedEntry('verified_chmod',
-                                     ansible_test_only=True),
+        'os.chmod': UnwantedEntry('verified_chmod', ansible_test_only=True),
 
         'sys.exit': UnwantedEntry('exit_json or fail_json',
                                   ignore_paths=(
@@ -131,6 +129,33 @@ class AnsibleUnwantedChecker(BaseChecker):
                                         modules_only=True),
     }
 
+    for method in (
+        'popen',
+        'posix_spawn',
+        'posix_spawnp',
+        'spawnl',
+        'spawnle',
+        'spawnlp',
+        'spawnlpe',
+        'spawnv',
+        'spawnve',
+        'spawnvp',
+        'spawnvpe',
+        'system',
+    ):
+        unwanted_functions[f'os.{method}'] = UnwantedEntry('run_command', modules_only=True)
+
+    for method in (
+        'Popen',
+        'call',
+        'check_call',
+        'check_output',
+        'getoutput',
+        'getstatusoutput',
+        'run',
+    ):
+        unwanted_functions[f'subprocess.{method}'] = UnwantedEntry('run_command', modules_only=True)
+
     @functools.cached_property
     def is_ansible_core(self) -> bool:
         """True if ansible-core is being tested."""
@@ -167,8 +192,18 @@ class AnsibleUnwantedChecker(BaseChecker):
             for i in node.func.inferred():
                 func = None
 
-                if isinstance(i, astroid.nodes.FunctionDef) and isinstance(i.parent, astroid.nodes.Module):
-                    func = '%s.%s' % (i.parent.name, i.name)
+                if isinstance(i.parent, astroid.nodes.Module):
+                    parent_module = i.parent.name
+                elif isinstance(i.parent, astroid.nodes.If) and isinstance(i.parent.parent, astroid.nodes.Module):
+                    parent_module = i.parent.parent.name
+                else:
+                    parent_module = None
+
+                if parent_module == 'posix':
+                    parent_module = 'os'  # some os.* functions we're looking for show up as posix.* imports
+
+                if parent_module and isinstance(i, (astroid.nodes.FunctionDef, astroid.nodes.ClassDef)):
+                    func = f'{parent_module}.{i.name}'
 
                 if not func:
                     continue

test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/constants.py

@@ -58,8 +58,6 @@ REJECTLIST_IMPORTS = {
         }
     },
 }
-SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*')
-OS_CALL_REGEX = re.compile(r'os\.call.*')
 
 
 PLUGINS_WITH_RETURN_VALUES = ('module', )

test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py

@@ -91,8 +91,6 @@ from .constants import (
     NO_LOG_REGEX,
     FORBIDDEN_DICTIONARY_KEYS,
     REJECTLIST_IMPORTS,
-    SUBPROCESS_REGEX,
-    OS_CALL_REGEX,
     PLUGINS_WITH_RETURN_VALUES,
     PLUGINS_WITH_EXAMPLES,
     PLUGINS_WITH_YAML_EXAMPLES,
@@ -455,34 +453,6 @@ class ModuleValidator(Validator):
                         'https://docs.ansible.com/ansible-core/devel/dev_guide/developing_modules_documenting.html#copyright'
                 )
 
-    def _check_for_subprocess(self):
-        for child in self.ast.body:
-            if isinstance(child, ast.Import):
-                if child.names[0].name == 'subprocess':
-                    for line_no, line in enumerate(self.text.splitlines()):
-                        sp_match = SUBPROCESS_REGEX.search(line)
-                        if sp_match:
-                            self.reporter.error(
-                                path=self.object_path,
-                                code='use-run-command-not-popen',
-                                msg=('subprocess.Popen call found. Should be module.run_command'),
-                                line=(line_no + 1),
-                                column=(sp_match.span()[0] + 1)
-                            )
-
-    def _check_for_os_call(self):
-        if 'os.call' in self.text:
-            for line_no, line in enumerate(self.text.splitlines()):
-                os_call_match = OS_CALL_REGEX.search(line)
-                if os_call_match:
-                    self.reporter.error(
-                        path=self.object_path,
-                        code='use-run-command-not-os-call',
-                        msg=('os.call() call found. Should be module.run_command'),
-                        line=(line_no + 1),
-                        column=(os_call_match.span()[0] + 1)
-                    )
-
     def _find_rejectlist_imports(self):
         for child in self.ast.body:
             names = []
@@ -2445,10 +2415,6 @@ class ModuleValidator(Validator):
                 first_callable = self._get_first_callable() or 1000000  # use a bogus "high" line number if no callable exists
                 self._ensure_imports_below_docs(doc_info, first_callable)
 
-            if self.plugin_type == 'module':
-                self._check_for_subprocess()
-                self._check_for_os_call()
-
         if self._powershell_module():
             self._validate_ps_replacers()
             docs_path = self._find_ps_docs_file()

test/sanity/ignore.txt

@@ -23,11 +23,17 @@ lib/ansible/modules/git.py use-argspec-type-path
 lib/ansible/modules/git.py validate-modules:doc-required-mismatch
 lib/ansible/modules/package_facts.py validate-modules:doc-choices-do-not-match-spec
 lib/ansible/modules/service.py validate-modules:nonexistent-parameter-documented
-lib/ansible/modules/service.py validate-modules:use-run-command-not-popen
 lib/ansible/modules/stat.py validate-modules:parameter-invalid
 lib/ansible/modules/systemd_service.py validate-modules:parameter-invalid
 lib/ansible/modules/user.py validate-modules:doc-default-does-not-match-spec
-lib/ansible/modules/user.py validate-modules:use-run-command-not-popen
+lib/ansible/module_utils/basic.py pylint:ansible-bad-function
+lib/ansible/module_utils/common/respawn.py pylint:ansible-bad-function
+lib/ansible/module_utils/distro/_distro.py pylint:ansible-bad-function
+lib/ansible/module_utils/facts/system/pkg_mgr.py pylint:ansible-bad-function
+lib/ansible/module_utils/service.py pylint:ansible-bad-function
+lib/ansible/modules/mount_facts.py pylint:ansible-bad-function
+lib/ansible/modules/service.py pylint:ansible-bad-function
+lib/ansible/modules/user.py pylint:ansible-bad-function
 lib/ansible/module_utils/basic.py pylint:unused-import  # deferring resolution to allow enabling the rule now
 lib/ansible/module_utils/compat/selinux.py import-3.9!skip # pass/fail depends on presence of libselinux.so
 lib/ansible/module_utils/compat/selinux.py import-3.10!skip # pass/fail depends on presence of libselinux.so