From a342538aba46b3b9b63700daace6b6dc9bea805f Mon Sep 17 00:00:00 2001 From: Andrea Tartaglia Date: Thu, 21 Jun 2018 16:58:39 +0100 Subject: [PATCH] Add shell out checks (#41545) * Added error codes for shell_out checks * Added ignore lines for allowed Modules * Added shell out checks * Fixed pep8 * Updated regex to only match subprocess.Popen * Added failing modules to ignore.txt * Wrong postgresql module in ignore.txt * Removed bigip from ignore.txt --- .../dev_guide/testing_validate-modules.rst | 2 ++ test/sanity/validate-modules/ignore.txt | 6 ++++ test/sanity/validate-modules/main.py | 33 ++++++++++++++++++- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/docs/docsite/rst/dev_guide/testing_validate-modules.rst b/docs/docsite/rst/dev_guide/testing_validate-modules.rst index 2c751e99498..34224939158 100644 --- a/docs/docsite/rst/dev_guide/testing_validate-modules.rst +++ b/docs/docsite/rst/dev_guide/testing_validate-modules.rst @@ -83,6 +83,8 @@ Errors 208 ``module_utils`` imports should import specific components, not ``*`` 209 Only the following ``from __future__`` imports are allowed: ``absolute_import``, ``division``, and ``print_function``. + 210 ``subprocess.Popen`` used instead of ``module.run_command`` + 211 ``os.call`` used instead of ``module.run_command`` .. --------- ------------------- **3xx** **Documentation** diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt index 49a64f5d4ff..01dbeef19b4 100644 --- a/test/sanity/validate-modules/ignore.txt +++ b/test/sanity/validate-modules/ignore.txt @@ -251,6 +251,7 @@ lib/ansible/modules/cloud/linode/linode.py E322 lib/ansible/modules/cloud/linode/linode.py E324 lib/ansible/modules/cloud/lxc/lxc_container.py E324 lib/ansible/modules/cloud/lxc/lxc_container.py E326 +lib/ansible/modules/cloud/lxc/lxc_container.py E210 lib/ansible/modules/cloud/lxd/lxd_container.py E322 lib/ansible/modules/cloud/lxd/lxd_container.py E324 lib/ansible/modules/cloud/lxd/lxd_container.py E325 @@ -565,6 +566,7 @@ lib/ansible/modules/database/mongodb/mongodb_user.py E325 lib/ansible/modules/database/mysql/mysql_replication.py E325 lib/ansible/modules/database/mysql/mysql_replication.py E326 lib/ansible/modules/database/mysql/mysql_user.py E322 +lib/ansible/modules/database/mysql/mysql_db.py E210 lib/ansible/modules/database/postgresql/postgresql_ext.py E322 lib/ansible/modules/database/postgresql/postgresql_ext.py E324 lib/ansible/modules/database/postgresql/postgresql_lang.py E324 @@ -574,6 +576,7 @@ lib/ansible/modules/database/postgresql/postgresql_schema.py E324 lib/ansible/modules/database/postgresql/postgresql_user.py E322 lib/ansible/modules/database/postgresql/postgresql_user.py E324 lib/ansible/modules/database/postgresql/postgresql_user.py E326 +lib/ansible/modules/database/postgresql/postgresql_db.py E210 lib/ansible/modules/database/proxysql/proxysql_backend_servers.py E322 lib/ansible/modules/database/proxysql/proxysql_backend_servers.py E325 lib/ansible/modules/database/proxysql/proxysql_global_variables.py E322 @@ -1036,6 +1039,8 @@ lib/ansible/modules/packaging/os/apt_repository.py E325 lib/ansible/modules/packaging/os/apt_rpm.py E322 lib/ansible/modules/packaging/os/apt_rpm.py E324 lib/ansible/modules/packaging/os/apt_rpm.py E326 +lib/ansible/modules/packaging/os/flatpak.py E210 +lib/ansible/modules/packaging/os/flatpak_remote.py E210 lib/ansible/modules/packaging/os/homebrew.py E326 lib/ansible/modules/packaging/os/homebrew_cask.py E326 lib/ansible/modules/packaging/os/layman.py E322 @@ -1220,6 +1225,7 @@ lib/ansible/modules/system/selinux.py E324 lib/ansible/modules/system/selinux_permissive.py E322 lib/ansible/modules/system/seport.py E324 lib/ansible/modules/system/service.py E323 +lib/ansible/modules/system/service.py E210 lib/ansible/modules/system/solaris_zone.py E324 lib/ansible/modules/system/svc.py E322 lib/ansible/modules/system/svc.py E324 diff --git a/test/sanity/validate-modules/main.py b/test/sanity/validate-modules/main.py index e60c963b89a..6a63d0dd6b0 100755 --- a/test/sanity/validate-modules/main.py +++ b/test/sanity/validate-modules/main.py @@ -80,6 +80,8 @@ BLACKLIST_IMPORTS = { } }, } +SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*') +OS_CALL_REGEX = re.compile(r'os\.call.*') class ReporterEncoder(json.JSONEncoder): @@ -396,6 +398,34 @@ class ModuleValidator(Validator): 'https://docs.ansible.com/ansible/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=210, + 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=211, + msg=('os.call() call found. Should be module.run_command'), + line=(line_no + 1), + column=(os_call_match.span()[0] + 1) + ) + def _find_blacklist_imports(self): for child in self.ast.body: names = [] @@ -1308,7 +1338,6 @@ class ModuleValidator(Validator): def validate(self): super(ModuleValidator, self).validate() - if not self._python_module() and not self._powershell_module(): self.reporter.error( path=self.object_path, @@ -1354,6 +1383,8 @@ class ModuleValidator(Validator): self._find_has_import() first_callable = self._get_first_callable() self._ensure_imports_below_docs(doc_info, first_callable) + self._check_for_subprocess() + self._check_for_os_call() if self._powershell_module(): self._validate_ps_replacers()