From d9533c44aa4895ba9a3303926153e375d571b80b Mon Sep 17 00:00:00 2001 From: Alexei Znamensky <103110+russoz@users.noreply.github.com> Date: Sat, 31 Jul 2021 04:07:38 +1200 Subject: [PATCH] apache2_module - multiple improvements (#3106) * 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 * removed extraneous dependency in integration test Co-authored-by: Felix Fontein --- .../fragments/3106-apache2_module-review.yaml | 2 + .../web_infrastructure/apache2_module.py | 35 ++++----- .../apache2_module/tasks/actualtest.yml | 74 ++++++++----------- .../targets/apache2_module/tasks/main.yml | 24 +++++- 4 files changed, 71 insertions(+), 64 deletions(-) create mode 100644 changelogs/fragments/3106-apache2_module-review.yaml diff --git a/changelogs/fragments/3106-apache2_module-review.yaml b/changelogs/fragments/3106-apache2_module-review.yaml new file mode 100644 index 0000000000..d7840b2511 --- /dev/null +++ b/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). diff --git a/plugins/modules/web_infrastructure/apache2_module.py b/plugins/modules/web_infrastructure/apache2_module.py index 4cc0ef8b37..d85ed0158f 100644 --- a/plugins/modules/web_infrastructure/apache2_module.py +++ b/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']) diff --git a/tests/integration/targets/apache2_module/tasks/actualtest.yml b/tests/integration/targets/apache2_module/tasks/actualtest.yml index 24ba4f27cd..886e746f07 100644 --- a/tests/integration/targets/apache2_module/tasks/actualtest.yml +++ b/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 . -- 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 diff --git a/tests/integration/targets/apache2_module/tasks/main.yml b/tests/integration/targets/apache2_module/tasks/main.yml index 2ec308857a..d840ff60e8 100644 --- a/tests/integration/targets/apache2_module/tasks/main.yml +++ b/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