apache2_module - multiple improvements (#3106) (#3116)

* multiple improvements

* added changelog fragment

* comment and name in int test files

* added notes to the documentation

* removed the extraneous changelog frag

* Update plugins/modules/web_infrastructure/apache2_module.py

* adjusted doc text for sanity check

* Update plugins/modules/web_infrastructure/apache2_module.py

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

* removed extraneous dependency in integration test

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit d9533c44aa)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

changelogs/fragments/3106-apache2_module-review.yaml

@@ -0,0 +1,2 @@
+minor_changes:
+  - apache2_module - minor refactoring improving code quality, readability and speed (https://github.com/ansible-collections/community.general/pull/3106).

plugins/modules/web_infrastructure/apache2_module.py

@@ -49,6 +49,9 @@ options:
      type: bool
      default: False
 requirements: ["a2enmod","a2dismod"]
+notes:
+  - This does not work on RedHat-based distributions. It does work on Debian- and SuSE-based distributions.
+    Whether it works on others depend on whether the C(a2enmod) and C(a2dismod) tools are available or not.
 '''
 
 EXAMPLES = '''
@@ -109,13 +112,14 @@ import re
 # import module snippets
 from ansible.module_utils.basic import AnsibleModule
 
+_re_threaded = re.compile(r'threaded: *yes')
+
 
 def _run_threaded(module):
     control_binary = _get_ctl_binary(module)
+    result, stdout, stderr = module.run_command([control_binary, "-V"])
 
-    result, stdout, stderr = module.run_command("%s -V" % control_binary)
-
-    return bool(re.search(r'threaded:[ ]*yes', stdout))
+    return bool(_re_threaded.search(stdout))
 
 
 def _get_ctl_binary(module):
@@ -124,15 +128,12 @@ def _get_ctl_binary(module):
         if ctl_binary is not None:
             return ctl_binary
 
-    module.fail_json(
-        msg="Neither of apache2ctl nor apachctl found."
-            " At least one apache control binary is necessary."
-    )
+    module.fail_json(msg="Neither of apache2ctl nor apachctl found. At least one apache control binary is necessary.")
 
 
 def _module_is_enabled(module):
     control_binary = _get_ctl_binary(module)
-    result, stdout, stderr = module.run_command("%s -M" % control_binary)
+    result, stdout, stderr = module.run_command([control_binary, "-M"])
 
     if result != 0:
         error_msg = "Error executing %s: %s" % (control_binary, stderr)
@@ -168,7 +169,7 @@ def create_apache_identifier(name):
 
     # re expressions to extract subparts of names
     re_workarounds = [
-        ('php', r'^(php\d)\.'),
+        ('php', re.compile(r'^(php\d)\.')),
     ]
 
     for a2enmod_spelling, module_name in text_workarounds:
@@ -178,7 +179,7 @@ def create_apache_identifier(name):
     for search, reexpr in re_workarounds:
         if search in name:
             try:
-                rematch = re.search(reexpr, name)
+                rematch = reexpr.search(name)
                 return rematch.group(1) + '_module'
             except AttributeError:
                 pass
@@ -201,15 +202,15 @@ def _set_state(module, state):
                              result=success_msg,
                              warnings=module.warnings)
 
-        a2mod_binary = module.get_bin_path(a2mod_binary)
+        a2mod_binary = [module.get_bin_path(a2mod_binary)]
         if a2mod_binary is None:
             module.fail_json(msg="%s not found. Perhaps this system does not use %s to manage apache" % (a2mod_binary, a2mod_binary))
 
         if not want_enabled and force:
             # force exists only for a2dismod on debian
-            a2mod_binary += ' -f'
+            a2mod_binary.append('-f')
 
-        result, stdout, stderr = module.run_command("%s %s" % (a2mod_binary, name))
+        result, stdout, stderr = module.run_command(a2mod_binary + [name])
 
         if _module_is_enabled(module) == want_enabled:
             module.exit_json(changed=True,
@@ -241,10 +242,10 @@ def main():
     module = AnsibleModule(
         argument_spec=dict(
             name=dict(required=True),
-            identifier=dict(required=False, type='str'),
-            force=dict(required=False, type='bool', default=False),
+            identifier=dict(type='str'),
+            force=dict(type='bool', default=False),
             state=dict(default='present', choices=['absent', 'present']),
-            ignore_configcheck=dict(required=False, type='bool', default=False),
+            ignore_configcheck=dict(type='bool', default=False),
         ),
         supports_check_mode=True,
     )
@@ -253,7 +254,7 @@ def main():
 
     name = module.params['name']
     if name == 'cgi' and _run_threaded(module):
-        module.fail_json(msg="Your MPM seems to be threaded. No automatic actions on module %s possible." % name)
+        module.fail_json(msg="Your MPM seems to be threaded. No automatic actions on module cgi possible.")
 
     if not module.params['identifier']:
         module.params['identifier'] = create_apache_identifier(module.params['name'])

tests/integration/targets/apache2_module/tasks/actualtest.yml

@@ -13,40 +13,25 @@
 # You should have received a copy of the GNU General Public License
 # along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
-- name: install apache via apt
-  apt:
-    name: "{{item}}"
-    state: present
-  when: "ansible_os_family == 'Debian'"
-  with_items:
-    - apache2
-    - libapache2-mod-evasive
-
-- name: install apache via zypper
-  community.general.zypper:
-    name: apache2
-    state: present
-  when: "ansible_os_family == 'Suse'"
-
 - name: disable userdir module
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: absent
   register: userdir_first_disable
 
 - name: disable userdir module, second run
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: absent
   register: disable
 
-- name: ensure apache2_module is idempotent
+- name: ensure community.general.apache2_module is idempotent
   assert:
     that:
       - disable is not changed
 
 - name: enable userdir module
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: present
   register: enable
@@ -57,18 +42,18 @@
       - enable is changed
 
 - name: enable userdir module, second run
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: present
   register: enabletwo
 
-- name: ensure apache2_module is idempotent
+- name: ensure community.general.apache2_module is idempotent
   assert:
     that:
       - 'not enabletwo.changed'
 
 - name: disable userdir module, final run
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: absent
   register: disablefinal
@@ -79,13 +64,13 @@
       - 'disablefinal.changed'
 
 - name: set userdir to original state
-  apache2_module:
+  community.general.apache2_module:
     name: userdir
     state: present
   when: userdir_first_disable is changed
 
 - name: ensure autoindex enabled
-  apache2_module:
+  community.general.apache2_module:
     name: autoindex
     state: present
 
@@ -93,55 +78,56 @@
   when: "ansible_os_family == 'Debian'"
   block:
   - name: force disable of autoindex  # bug #2499
-    apache2_module:
+    community.general.apache2_module:
       name: autoindex
       state: absent
       force: True
 
   - name: reenable autoindex
-    apache2_module:
+    community.general.apache2_module:
       name: autoindex
       state: present
 
-  - name: enable evasive module, test https://github.com/ansible/ansible/issues/22635
-    apache2_module:
-      name: evasive
-      state: present
-
+  # mod_evasive is enabled by default upon the installation, so disable first and enable second, to preserve the config
   - name: disable evasive module
-    apache2_module:
+    community.general.apache2_module:
       name: evasive
       state: absent
 
+  - name: enable evasive module, test https://github.com/ansible/ansible/issues/22635
+    community.general.apache2_module:
+      name: evasive
+      state: present
+
   - name: use identifier to enable module, fix for https://github.com/ansible/ansible/issues/33669
-    apache2_module:
+    community.general.apache2_module:
       name: dump_io
       state: present
     ignore_errors: True
     register: enable_dumpio_wrong
 
   - name: disable dump_io
-    apache2_module:
+    community.general.apache2_module:
       name: dump_io
       identifier: dumpio_module
       state: absent
 
   - name: use identifier to enable module, fix for https://github.com/ansible/ansible/issues/33669
-    apache2_module:
+    community.general.apache2_module:
       name: dump_io
       identifier: dumpio_module
       state: present
     register: enable_dumpio_correct_1
 
   - name: ensure idempotency with identifier
-    apache2_module:
+    community.general.apache2_module:
       name: dump_io
       identifier: dumpio_module
       state: present
     register: enable_dumpio_correct_2
 
   - name: disable dump_io
-    apache2_module:
+    community.general.apache2_module:
       name: dump_io
       identifier: dumpio_module
       state: absent
@@ -153,7 +139,7 @@
         - enable_dumpio_correct_2 is not changed
 
   - name: disable mpm modules
-    apache2_module:
+    community.general.apache2_module:
       name: "{{ item }}"
       state: absent
       ignore_configcheck: True
@@ -163,7 +149,7 @@
       - mpm_prefork
 
   - name: enabled mpm_event
-    apache2_module:
+    community.general.apache2_module:
       name: mpm_event
       state: present
       ignore_configcheck: True
@@ -175,7 +161,7 @@
         - 'enabledmpmevent.changed'
 
   - name: switch between mpm_event and mpm_worker
-    apache2_module:
+    community.general.apache2_module:
       name: "{{ item.name }}"
       state: "{{ item.state }}"
       ignore_configcheck: True
@@ -186,7 +172,7 @@
         state: present
 
   - name: ensure mpm_worker is already enabled
-    apache2_module:
+    community.general.apache2_module:
       name: mpm_worker
       state: present
     register: enabledmpmworker
@@ -197,7 +183,7 @@
         - 'not enabledmpmworker.changed'
 
   - name: try to disable all mpm modules with configcheck
-    apache2_module:
+    community.general.apache2_module:
       name: "{{item}}"
       state: absent
     with_items:
@@ -214,7 +200,7 @@
     with_items: "{{ remove_with_configcheck.results }}"
 
   - name: try to disable all mpm modules without configcheck
-    apache2_module:
+    community.general.apache2_module:
       name: "{{item}}"
       state: absent
       ignore_configcheck: True
@@ -224,7 +210,7 @@
       - mpm_prefork
 
   - name: enabled mpm_event to restore previous state
-    apache2_module:
+    community.general.apache2_module:
       name: mpm_event
       state: present
       ignore_configcheck: True

tests/integration/targets/apache2_module/tasks/main.yml

@@ -5,8 +5,22 @@
 ####################################################################
 
 
+- name: install apache via apt
+  apt:
+    name: "{{item}}"
+    state: present
+  when: "ansible_os_family == 'Debian'"
+  with_items:
+    - apache2
+    - libapache2-mod-evasive
 
-- name:
+- name: install apache via zypper
+  community.general.zypper:
+    name: apache2
+    state: present
+  when: "ansible_os_family == 'Suse'"
+
+- name: test apache2_module
   block:
     - name: get list of enabled modules
       shell: apache2ctl -M | sort
@@ -17,8 +31,12 @@
     - name: get list of enabled modules
       shell: apache2ctl -M | sort
       register: modules_after
-    - debug: var=modules_before
-    - debug: var=modules_after
+    - name: modules_before
+      debug:
+        var: modules_before
+    - name: modules_after
+      debug:
+        var: modules_after
     - name: ensure that all test modules are disabled again
       assert:
         that: modules_before.stdout == modules_after.stdout