Applying ModuleHelper variable mgmt to xfconf -> improvements on MH (#2188)

* applying MH variable mgmt to xfconf - improvements on MH

* added changelog fragment

changelogs/fragments/2188-xfconf-modhelper-variables.yml

@@ -0,0 +1,3 @@
+minor_changes:
+  - module_helper module utils - added management of facts and adhoc setting of the initial value for variables (https://github.com/ansible-collections/community.general/pull/2188).
+  - xfconf - changed implementation to use ``ModuleHelper`` new features (https://github.com/ansible-collections/community.general/pull/2188).

plugins/module_utils/module_helper.py

@@ -159,7 +159,9 @@ class DependencyCtxMgr(object):
 
 
 class VarMeta(object):
-    def __init__(self, diff=False, output=False, change=None):
+    NOTHING = object()
+
+    def __init__(self, diff=False, output=True, change=None, fact=False):
         self.init = False
         self.initial_value = None
         self.value = None
@@ -167,14 +169,19 @@ class VarMeta(object):
         self.diff = diff
         self.change = diff if change is None else change
         self.output = output
+        self.fact = fact
 
-    def set(self, diff=None, output=None, change=None):
+    def set(self, diff=None, output=None, change=None, fact=None, initial_value=NOTHING):
         if diff is not None:
             self.diff = diff
         if output is not None:
             self.output = output
         if change is not None:
             self.change = change
+        if fact is not None:
+            self.fact = fact
+        if initial_value is not self.NOTHING:
+            self.initial_value = initial_value
 
     def set_value(self, value):
         if not self.init:
@@ -208,6 +215,7 @@ class ModuleHelper(object):
     output_params = ()
     diff_params = ()
     change_params = ()
+    facts_params = ()
 
     class VarDict(object):
         def __init__(self):
@@ -245,8 +253,6 @@ class ModuleHelper(object):
             if name in self._meta:
                 meta = self.meta(name)
             else:
-                if 'output' not in kwargs:
-                    kwargs['output'] = True
                 meta = VarMeta(**kwargs)
             meta.set_value(value)
             self._meta[name] = meta
@@ -261,9 +267,12 @@ class ModuleHelper(object):
                 before = dict((dr[0], dr[1]['before']) for dr in diff_results)
                 after = dict((dr[0], dr[1]['after']) for dr in diff_results)
                 return {'before': before, 'after': after}
-
             return None
 
+        def facts(self):
+            facts_result = dict((k, v) for k, v in self._data.items() if self._meta[k].fact)
+            return facts_result if facts_result else None
+
         def change_vars(self):
             return [v for v in self._data if self.meta(v).change]
 
@@ -272,8 +281,6 @@ class ModuleHelper(object):
 
     def __init__(self, module=None):
         self.vars = ModuleHelper.VarDict()
-        self.output_dict = dict()
-        self.facts_dict = dict()
         self._changed = False
 
         if module:
@@ -288,13 +295,20 @@ class ModuleHelper(object):
                 diff=name in self.diff_params,
                 output=name in self.output_params,
                 change=None if not self.change_params else name in self.change_params,
+                fact=name in self.facts_params,
             )
 
+    def update_vars(self, meta=None, **kwargs):
+        if meta is None:
+            meta = {}
+        for k, v in kwargs.items():
+            self.vars.set(k, v, **meta)
+
     def update_output(self, **kwargs):
-        self.output_dict.update(kwargs)
+        self.update_vars(meta={"output": True}, **kwargs)
 
     def update_facts(self, **kwargs):
-        self.facts_dict.update(kwargs)
+        self.update_vars(meta={"fact": True}, **kwargs)
 
     def __init_module__(self):
         pass
@@ -322,9 +336,10 @@ class ModuleHelper(object):
     @property
     def output(self):
         result = dict(self.vars.output())
-        result.update(self.output_dict)
         if self.facts_name:
-            result['ansible_facts'] = {self.facts_name: self.facts_dict}
+            facts = self.vars.facts()
+            if facts is not None:
+                result['ansible_facts'] = {self.facts_name: facts}
         if self.module._diff:
             diff = result.get('diff', {})
             vars_diff = self.vars.diff() or {}

plugins/modules/system/xfconf.py

@@ -143,10 +143,7 @@ def values_fmt(values, value_types):
     for value, value_type in zip(values, value_types):
         if value_type == 'bool':
             value = fix_bool(value)
-        result.append('--type')
+        result.extend(['--type', '{0}'.format(value_type), '--set', '{0}'.format(value)])
-        result.append('{0}'.format(value_type))
-        result.append('--set')
-        result.append('{0}'.format(value))
     return result
 
 
@@ -155,6 +152,10 @@ class XFConfException(Exception):
 
 
 class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
+    change_params = 'value',
+    diff_params = 'value',
+    output_params = ('property', 'channel', 'value')
+    facts_params = ('property', 'channel', 'value')
     module = dict(
         argument_spec=dict(
             state=dict(default="present",
@@ -185,17 +186,15 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
     )
 
     def update_xfconf_output(self, **kwargs):
-        self.update_output(**kwargs)
+        self.update_vars(meta={"output": True, "fact": True}, **kwargs)
-        if not self.module.params['disable_facts']:
-            self.update_facts(**kwargs)
 
     def __init_module__(self):
         self.does_not = 'Property "{0}" does not exist on channel "{1}".'.format(self.module.params['property'],
                                                                                  self.module.params['channel'])
-        self.vars.previous_value = self._get()
+        self.vars.set('previous_value', self._get(), fact=True)
-        self.update_xfconf_output(property=self.module.params['property'],
+        self.vars.set('type', self.vars.value_type, fact=True)
-                                  channel=self.module.params['channel'],
+        self.vars.meta('value').set(initial_value=self.vars.previous_value)
-                                  previous_value=None)
+
         if not self.module.params['disable_facts']:
             self.facts_name = "xfconf"
             self.module.deprecate(
@@ -220,34 +219,23 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
 
         return result
 
-    @property
-    def changed(self):
-        if self.vars.previous_value is None:
-            return self.vars.value is not None
-        elif self.vars.value is None:
-            return self.vars.previous_value is not None
-        else:
-            return set(self.vars.previous_value) != set(self.vars.value)
-
     def _get(self):
         return self.run_command(params=('channel', 'property'))
 
     def state_get(self):
         self.vars.value = self.vars.previous_value
-        self.update_xfconf_output(value=self.vars.value)
+        self.vars.previous_value = None
 
     def state_absent(self):
         if not self.module.check_mode:
             self.run_command(params=('channel', 'property', {'reset': True}))
         self.vars.value = None
-        self.update_xfconf_output(previous_value=self.vars.previous_value,
-                                  value=None)
 
     def state_present(self):
         # stringify all values - in the CLI they will all be happy strings anyway
         # and by doing this here the rest of the code can be agnostic to it
-        self.vars.value = [str(v) for v in self.module.params['value']]
+        self.vars.value = [str(v) for v in self.vars.value]
-        value_type = self.module.params['value_type']
+        value_type = self.vars.value_type
 
         values_len = len(self.vars.value)
         types_len = len(value_type)
@@ -264,7 +252,7 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
 
         # calculates if it is an array
         self.vars.is_array = \
-            bool(self.module.params['force_array']) or \
+            bool(self.vars.force_array) or \
             isinstance(self.vars.previous_value, list) or \
             values_len > 1
 
@@ -278,11 +266,9 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
 
         if not self.vars.is_array:
             self.vars.value = self.vars.value[0]
-            value_type = value_type[0]
+            self.vars.type = value_type[0]
-
+        else:
-        self.update_xfconf_output(previous_value=self.vars.previous_value,
+            self.vars.type = value_type
-                                  value=self.vars.value,
-                                  type=value_type)
 
 
 def main():

tests/unit/plugins/module_utils/test_module_helper.py

@@ -109,7 +109,7 @@ def test_dependency_ctxmgr():
 
 def test_variable_meta():
     meta = VarMeta()
-    assert meta.output is False
+    assert meta.output is True
     assert meta.diff is False
     assert meta.value is None
     meta.set_value("abc")
@@ -124,7 +124,7 @@ def test_variable_meta():
 
 def test_variable_meta_diff():
     meta = VarMeta(diff=True)
-    assert meta.output is False
+    assert meta.output is True
     assert meta.diff is True
     assert meta.value is None
     meta.set_value("abc")