From b52a6f3611eb5bff023e1aca76e12c01e20786bd Mon Sep 17 00:00:00 2001 From: "patchback[bot]" <45432694+patchback[bot]@users.noreply.github.com> Date: Wed, 23 Nov 2022 07:37:27 +0100 Subject: [PATCH] gconftool2: refactored to use ModuleHelper + CmdRunner (#5545) (#5551) * gconftool2: refactored to use ModuleHelper + CmdRunner * add changelog fragment * removed old code commented out (cherry picked from commit 6c7e9116e1abba6c125e7f05cf7fd0e8042a1f43) Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com> --- .../fragments/5545-gconftool-cmd-runner.yml | 2 + plugins/module_utils/gconftool2.py | 23 ++- plugins/modules/gconftool2.py | 173 +++++------------- plugins/modules/gconftool2_info.py | 4 +- tests/sanity/ignore-2.11.txt | 2 +- tests/sanity/ignore-2.12.txt | 2 +- tests/sanity/ignore-2.13.txt | 2 +- tests/sanity/ignore-2.14.txt | 2 +- tests/sanity/ignore-2.15.txt | 2 +- tests/unit/plugins/modules/test_gconftool2.py | 116 ++++++++++++ 10 files changed, 186 insertions(+), 142 deletions(-) create mode 100644 changelogs/fragments/5545-gconftool-cmd-runner.yml create mode 100644 tests/unit/plugins/modules/test_gconftool2.py diff --git a/changelogs/fragments/5545-gconftool-cmd-runner.yml b/changelogs/fragments/5545-gconftool-cmd-runner.yml new file mode 100644 index 0000000000..a41d5c3657 --- /dev/null +++ b/changelogs/fragments/5545-gconftool-cmd-runner.yml @@ -0,0 +1,2 @@ +minor_changes: + - gconftool2 - refactor using ``ModuleHelper`` and ``CmdRunner`` (https://github.com/ansible-collections/community.general/pull/5545). diff --git a/plugins/module_utils/gconftool2.py b/plugins/module_utils/gconftool2.py index cd9de57695..e90c3fb2cb 100644 --- a/plugins/module_utils/gconftool2.py +++ b/plugins/module_utils/gconftool2.py @@ -6,7 +6,14 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type -from ansible_collections.community.general.plugins.module_utils.cmd_runner import CmdRunner, cmd_runner_fmt as fmt +from ansible_collections.community.general.plugins.module_utils.cmd_runner import CmdRunner, cmd_runner_fmt + + +_state_map = { + "present": "--set", + "absent": "--unset", + "get": "--get", +} def gconftool2_runner(module, **kwargs): @@ -14,14 +21,12 @@ def gconftool2_runner(module, **kwargs): module, command='gconftool-2', arg_formats=dict( - key=fmt.as_list(), - value_type=fmt.as_opt_val("--type"), - value=fmt.as_list(), - direct=fmt.as_bool("--direct"), - config_source=fmt.as_opt_val("--config-source"), - get=fmt.as_bool("--get"), - set_arg=fmt.as_bool("--set"), - unset=fmt.as_bool("--unset"), + state=cmd_runner_fmt.as_map(_state_map), + key=cmd_runner_fmt.as_list(), + value_type=cmd_runner_fmt.as_opt_val("--type"), + value=cmd_runner_fmt.as_list(), + direct=cmd_runner_fmt.as_bool("--direct"), + config_source=cmd_runner_fmt.as_opt_val("--config-source"), ), **kwargs ) diff --git a/plugins/modules/gconftool2.py b/plugins/modules/gconftool2.py index 931b43d76c..a3ac8bb8f1 100644 --- a/plugins/modules/gconftool2.py +++ b/plugins/modules/gconftool2.py @@ -83,75 +83,17 @@ RETURN = ''' ... ''' -from ansible.module_utils.basic import AnsibleModule +from ansible_collections.community.general.plugins.module_utils.module_helper import StateModuleHelper +from ansible_collections.community.general.plugins.module_utils.gconftool2 import gconftool2_runner -class GConf2Preference(object): - def __init__(self, ansible, key, value_type, value, - direct=False, config_source=""): - self.ansible = ansible - self.key = key - self.value_type = value_type - self.value = value - self.config_source = config_source - self.direct = direct - - def value_already_set(self): - return False - - def call(self, call_type, fail_onerr=True): - """ Helper function to perform gconftool-2 operations """ - config_source = [] - direct = [] - changed = False - out = '' - - # If the configuration source is different from the default, create - # the argument - if self.config_source is not None and len(self.config_source) > 0: - config_source = ["--config-source", self.config_source] - - # If direct is true, create the argument - if self.direct: - direct = ["--direct"] - - # Execute the call - cmd = ["gconftool-2"] - try: - # If the call is "get", then we don't need as many parameters and - # we can ignore some - if call_type == 'get': - self.ansible.deprecate( - msg="State 'get' is deprecated. Please use the module community.general.gconftool2_info instead", - version="8.0.0", collection_name="community.general" - ) - cmd.extend(["--get", self.key]) - # Otherwise, we will use all relevant parameters - elif call_type == 'set': - cmd.extend(direct) - cmd.extend(config_source) - cmd.extend(["--type", self.value_type, "--{3}".format(call_type), self.key, self.value]) - elif call_type == 'unset': - cmd.extend(["--unset", self.key]) - - # Start external command - rc, out, err = self.ansible.run_command(cmd) - - if err and fail_onerr: - self.ansible.fail_json(msg='gconftool-2 failed with ' - 'error: %s' % (str(err))) - else: - changed = True - - except OSError as exception: - self.ansible.fail_json(msg='gconftool-2 failed with exception: ' - '%s' % exception) - return changed, out.rstrip() - - -def main(): - # Setup the Ansible module - module = AnsibleModule( +class GConftool(StateModuleHelper): + change_params = 'value', + diff_params = 'value', + output_params = ('key', 'value_type') + facts_params = ('key', 'value_type') + facts_name = 'gconftool2' + module = dict( argument_spec=dict( key=dict(type='str', required=True, no_log=False), value_type=dict(type='str', choices=['bool', 'float', 'int', 'string']), @@ -160,75 +102,54 @@ def main(): direct=dict(type='bool', default=False), config_source=dict(type='str'), ), - supports_check_mode=True + required_if=[ + ('state', 'present', ['value', 'value_type']), + ('state', 'absent', ['value']), + ('direct', True, ['config_source']), + ], + supports_check_mode=True, ) - state_values = {"present": "set", "absent": "unset", "get": "get"} + def __init_module__(self): + self.runner = gconftool2_runner(self.module, check_rc=True) + if self.vars.state != "get": + if not self.vars.direct and self.vars.config_source is not None: + self.module.fail_json(msg='If the "config_source" is specified then "direct" must be "true"') - # Assign module values to dictionary values - key = module.params['key'] - value_type = module.params['value_type'] - if module.params['value'].lower() == "true": - value = "true" - elif module.params['value'] == "false": - value = "false" - else: - value = module.params['value'] + self.vars.set('previous_value', self._get(), fact=True) + self.vars.set('value_type', self.vars.value_type) + self.vars.set_meta('value', initial_value=self.vars.previous_value) + self.vars.set('playbook_value', self.vars.value, fact=True) - state = state_values[module.params['state']] - direct = module.params['direct'] - config_source = module.params['config_source'] + def _make_process(self, fail_on_err): + def process(rc, out, err): + if err and fail_on_err: + self.ansible.fail_json(msg='gconftool-2 failed with error: %s' % (str(err))) + self.vars.value = out.rstrip() + return self.vars.value + return process - # Initialize some variables for later - change = False - new_value = '' + def _get(self): + return self.runner("state key", output_process=self._make_process(False)).run(state="get") - if state != "get": - if value is None or value == "": - module.fail_json(msg='State %s requires "value" to be set' - % str(state)) - elif value_type is None or value_type == "": - module.fail_json(msg='State %s requires "value_type" to be set' - % str(state)) + def state_get(self): + self.deprecate( + msg="State 'get' is deprecated. Please use the module community.general.gconftool2_info instead", + version="8.0.0", collection_name="community.general" + ) - if direct and config_source is None: - module.fail_json(msg='If "direct" is "true" then the ' + - '"config_source" must be specified') - elif not direct and config_source is not None: - module.fail_json(msg='If the "config_source" is specified ' + - 'then "direct" must be "true"') + def state_absent(self): + with self.runner("state key", output_process=self._make_process(False)) as ctx: + ctx.run() + self.vars.set('new_value', None, fact=True) - # Create a gconf2 preference - gconf_pref = GConf2Preference(module, key, value_type, - value, direct, config_source) - # Now we get the current value, if not found don't fail - dummy, current_value = gconf_pref.call("get", fail_onerr=False) + def state_present(self): + with self.runner("direct config_source value_type state key value", output_process=self._make_process(True)) as ctx: + self.vars.set('new_value', ctx.run(), fact=True) - # Check if the current value equals the value we want to set. If not, make - # a change - if current_value != value: - # If check mode, we know a change would have occurred. - if module.check_mode: - # So we will set the change to True - change = True - # And set the new_value to the value that would have been set - new_value = value - # If not check mode make the change. - else: - change, new_value = gconf_pref.call(state) - # If the value we want to set is the same as the current_value, we will - # set the new_value to the current_value for reporting - else: - new_value = current_value - facts = dict(gconftool2={'changed': change, - 'key': key, - 'value_type': value_type, - 'new_value': new_value, - 'previous_value': current_value, - 'playbook_value': module.params['value']}) - - module.exit_json(changed=change, ansible_facts=facts) +def main(): + GConftool.execute() if __name__ == '__main__': diff --git a/plugins/modules/gconftool2_info.py b/plugins/modules/gconftool2_info.py index f9231104d4..282065b95e 100644 --- a/plugins/modules/gconftool2_info.py +++ b/plugins/modules/gconftool2_info.py @@ -65,8 +65,8 @@ class GConftoolInfo(ModuleHelper): self.runner = gconftool2_runner(self.module, check_rc=True) def __run__(self): - with self.runner.context(args_order=["get", "key"]) as ctx: - rc, out, err = ctx.run(get=True) + with self.runner.context(args_order=["state", "key"]) as ctx: + rc, out, err = ctx.run(state="get") self.vars.value = None if err and not out else out.rstrip() diff --git a/tests/sanity/ignore-2.11.txt b/tests/sanity/ignore-2.11.txt index a736032da2..40b96de649 100644 --- a/tests/sanity/ignore-2.11.txt +++ b/tests/sanity/ignore-2.11.txt @@ -7,7 +7,7 @@ plugins/modules/consul.py validate-modules:doc-missing-type plugins/modules/consul.py validate-modules:undocumented-parameter plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choice -plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice +plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice # state=get - removed in 8.0.0 plugins/modules/iptables_state.py validate-modules:undocumented-parameter plugins/modules/jenkins_plugin.py use-argspec-type-path plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen diff --git a/tests/sanity/ignore-2.12.txt b/tests/sanity/ignore-2.12.txt index d01077cc9c..6e27e3105a 100644 --- a/tests/sanity/ignore-2.12.txt +++ b/tests/sanity/ignore-2.12.txt @@ -2,7 +2,7 @@ plugins/modules/consul.py validate-modules:doc-missing-type plugins/modules/consul.py validate-modules:undocumented-parameter plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choice -plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice +plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice # state=get - removed in 8.0.0 plugins/modules/iptables_state.py validate-modules:undocumented-parameter plugins/modules/jenkins_plugin.py use-argspec-type-path plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen diff --git a/tests/sanity/ignore-2.13.txt b/tests/sanity/ignore-2.13.txt index d01077cc9c..6e27e3105a 100644 --- a/tests/sanity/ignore-2.13.txt +++ b/tests/sanity/ignore-2.13.txt @@ -2,7 +2,7 @@ plugins/modules/consul.py validate-modules:doc-missing-type plugins/modules/consul.py validate-modules:undocumented-parameter plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choice -plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice +plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice # state=get - removed in 8.0.0 plugins/modules/iptables_state.py validate-modules:undocumented-parameter plugins/modules/jenkins_plugin.py use-argspec-type-path plugins/modules/lxc_container.py validate-modules:use-run-command-not-popen diff --git a/tests/sanity/ignore-2.14.txt b/tests/sanity/ignore-2.14.txt index 293e878680..da21cc0225 100644 --- a/tests/sanity/ignore-2.14.txt +++ b/tests/sanity/ignore-2.14.txt @@ -2,7 +2,7 @@ plugins/modules/consul.py validate-modules:doc-missing-type plugins/modules/consul.py validate-modules:undocumented-parameter plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choice -plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice +plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice # state=get - removed in 8.0.0 plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt' plugins/modules/iptables_state.py validate-modules:undocumented-parameter plugins/modules/jenkins_plugin.py use-argspec-type-path diff --git a/tests/sanity/ignore-2.15.txt b/tests/sanity/ignore-2.15.txt index 293e878680..da21cc0225 100644 --- a/tests/sanity/ignore-2.15.txt +++ b/tests/sanity/ignore-2.15.txt @@ -2,7 +2,7 @@ plugins/modules/consul.py validate-modules:doc-missing-type plugins/modules/consul.py validate-modules:undocumented-parameter plugins/modules/consul_session.py validate-modules:parameter-state-invalid-choice -plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice +plugins/modules/gconftool2.py validate-modules:parameter-state-invalid-choice # state=get - removed in 8.0.0 plugins/modules/homectl.py import-3.11 # Uses deprecated stdlib library 'crypt' plugins/modules/iptables_state.py validate-modules:undocumented-parameter plugins/modules/jenkins_plugin.py use-argspec-type-path diff --git a/tests/unit/plugins/modules/test_gconftool2.py b/tests/unit/plugins/modules/test_gconftool2.py new file mode 100644 index 0000000000..f01f15ef82 --- /dev/null +++ b/tests/unit/plugins/modules/test_gconftool2.py @@ -0,0 +1,116 @@ +# -*- coding: utf-8 -*- +# Copyright (c) Alexei Znamensky (russoz@gmail.com) +# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt) +# SPDX-License-Identifier: GPL-3.0-or-later + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +import json + +from ansible_collections.community.general.plugins.modules import gconftool2 + +import pytest + +TESTED_MODULE = gconftool2.__name__ + + +@pytest.fixture +def patch_gconftool2(mocker): + """ + Function used for mocking some parts of redhat_subscription module + """ + mocker.patch('ansible_collections.community.general.plugins.module_utils.mh.module_helper.AnsibleModule.get_bin_path', + return_value='/testbin/gconftool-2') + + +TEST_CASES = [ + [ + {'state': 'get', 'key': '/desktop/gnome/background/picture_filename'}, + { + 'id': 'test_simple_element_get', + 'run_command.calls': [ + ( + ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'], + {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True}, + (0, '100\n', '',), + ), + ], + 'new_value': '100', + } + ], + [ + {'state': 'get', 'key': '/desktop/gnome/background/picture_filename'}, + { + 'id': 'test_simple_element_get_not_found', + 'run_command.calls': [ + ( + ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'], + {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True}, + (0, '', "No value set for `/desktop/gnome/background/picture_filename'\n",), + ), + ], + 'new_value': None, + } + ], + [ + {'state': 'present', 'key': '/desktop/gnome/background/picture_filename', 'value': '200', 'value_type': 'int'}, + { + 'id': 'test_simple_element_set', + 'run_command.calls': [ + ( + ['/testbin/gconftool-2', '--get', '/desktop/gnome/background/picture_filename'], + {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True}, + (0, '100\n', '',), + ), + ( + ['/testbin/gconftool-2', '--type', 'int', '--set', '/desktop/gnome/background/picture_filename', '200'], + {'environ_update': {'LANGUAGE': 'C', 'LC_ALL': 'C'}, 'check_rc': True}, + (0, '200\n', '',), + ), + ], + 'new_value': '200', + } + ], +] +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_gconftool2(mocker, capfd, patch_gconftool2, testcase): + """ + Run unit tests for test cases listen in TEST_CASES + """ + + # Mock function used for running commands first + call_results = [item[2] for item in testcase['run_command.calls']] + mock_run_command = mocker.patch( + 'ansible_collections.community.general.plugins.module_utils.mh.module_helper.AnsibleModule.run_command', + side_effect=call_results) + + # Try to run test case + with pytest.raises(SystemExit): + gconftool2.main() + + out, err = capfd.readouterr() + results = json.loads(out) + print("testcase =\n%s" % testcase) + print("results =\n%s" % results) + + for conditional_test_result in ('value',): + if conditional_test_result in testcase: + assert conditional_test_result in results, "'{0}' not found in {1}".format(conditional_test_result, results) + assert results[conditional_test_result] == testcase[conditional_test_result], \ + "'{0}': '{1}' != '{2}'".format(conditional_test_result, results[conditional_test_result], testcase[conditional_test_result]) + + assert mock_run_command.call_count == len(testcase['run_command.calls']) + if mock_run_command.call_count: + call_args_list = [(item[0][0], item[1]) for item in mock_run_command.call_args_list] + expected_call_args_list = [(item[0], item[1]) for item in testcase['run_command.calls']] + print("call args list =\n%s" % call_args_list) + print("expected args list =\n%s" % expected_call_args_list) + assert call_args_list == expected_call_args_list