gconftool2: refactored to use ModuleHelper + CmdRunner (#5545)

* gconftool2: refactored to use ModuleHelper + CmdRunner

* add changelog fragment

* removed old code commented out

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).

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(),
+            state=cmd_runner_fmt.as_map(_state_map),
-            value_type=fmt.as_opt_val("--type"),
+            key=cmd_runner_fmt.as_list(),
-            value=fmt.as_list(),
+            value_type=cmd_runner_fmt.as_opt_val("--type"),
-            direct=fmt.as_bool("--direct"),
+            value=cmd_runner_fmt.as_list(),
-            config_source=fmt.as_opt_val("--config-source"),
+            direct=cmd_runner_fmt.as_bool("--direct"),
-            get=fmt.as_bool("--get"),
+            config_source=cmd_runner_fmt.as_opt_val("--config-source"),
-            set_arg=fmt.as_bool("--set"),
-            unset=fmt.as_bool("--unset"),
         ),
         **kwargs
     )

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):
+class GConftool(StateModuleHelper):
-    def __init__(self, ansible, key, value_type, value,
+    change_params = 'value',
-                 direct=False, config_source=""):
+    diff_params = 'value',
-        self.ansible = ansible
+    output_params = ('key', 'value_type')
-        self.key = key
+    facts_params = ('key', 'value_type')
-        self.value_type = value_type
+    facts_name = 'gconftool2'
-        self.value = value
+    module = dict(
-        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(
         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
+        self.vars.set('previous_value', self._get(), fact=True)
-    key = module.params['key']
+        self.vars.set('value_type', self.vars.value_type)
-    value_type = module.params['value_type']
+        self.vars.set_meta('value', initial_value=self.vars.previous_value)
-    if module.params['value'].lower() == "true":
+        self.vars.set('playbook_value', self.vars.value, fact=True)
-        value = "true"
-    elif module.params['value'] == "false":
-        value = "false"
-    else:
-        value = module.params['value']
 
-    state = state_values[module.params['state']]
+    def _make_process(self, fail_on_err):
-    direct = module.params['direct']
+        def process(rc, out, err):
-    config_source = module.params['config_source']
+            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
+    def _get(self):
-    change = False
+        return self.runner("state key", output_process=self._make_process(False)).run(state="get")
-    new_value = ''
 
-    if state != "get":
+    def state_get(self):
-        if value is None or value == "":
+        self.deprecate(
-            module.fail_json(msg='State %s requires "value" to be set'
+            msg="State 'get' is deprecated. Please use the module community.general.gconftool2_info instead",
-                                 % str(state))
+            version="8.0.0", collection_name="community.general"
-        elif value_type is None or value_type == "":
+        )
-            module.fail_json(msg='State %s requires "value_type" to be set'
-                                 % str(state))
 
-        if direct and config_source is None:
+    def state_absent(self):
-            module.fail_json(msg='If "direct" is "true" then the ' +
+        with self.runner("state key", output_process=self._make_process(False)) as ctx:
-                                 '"config_source" must be specified')
+            ctx.run()
-        elif not direct and config_source is not None:
+        self.vars.set('new_value', None, fact=True)
-            module.fail_json(msg='If the "config_source" is specified ' +
-                                 'then "direct" must be "true"')
 
-    # Create a gconf2 preference
+    def state_present(self):
-    gconf_pref = GConf2Preference(module, key, value_type,
+        with self.runner("direct config_source value_type state key value", output_process=self._make_process(True)) as ctx:
-                                  value, direct, config_source)
+            self.vars.set('new_value', ctx.run(), fact=True)
-    # Now we get the current value, if not found don't fail
-    dummy, current_value = gconf_pref.call("get", fail_onerr=False)
 
-    # 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,
+def main():
-                             'key': key,
+    GConftool.execute()
-                             '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)
 
 
 if __name__ == '__main__':

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:
+        with self.runner.context(args_order=["state", "key"]) as ctx:
-            rc, out, err = ctx.run(get=True)
+            rc, out, err = ctx.run(state="get")
             self.vars.value = None if err and not out else out.rstrip()
 
 

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

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

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

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

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

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