ModuleHelper variables management (#2162)

* added metadata for variables in module helper

* adjustments

* added separate support for tracking changes

* rewrote the diff code

* added integration test for module_helper

* using ansible.module_utils.common.dict_transformations.dict_merge

* improved dependency management

* restore ModuleHelper to base classes of CmdStateModuleHelper

* added assertions to ensure the failing module name appears in the error messages

* added test code for state-based modules

* fixed test name

* renamed class to VarMeta

* small fixes

* fixes from the PR

* fixed VarDict.__set_attr__

* added VarDict.__getitem__()

* added changelog fragment

* adjustments per PR

* ModuleHelper.output is now aware of conflicting variable names

* Update plugins/module_utils/module_helper.py

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

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

changelogs/fragments/2162-modhelper-variables.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - module_helper module utils - added mechanism to manage variables, providing automatic output of variables, change status and diff information (https://github.com/ansible-collections/community.general/pull/2162).

plugins/module_utils/module_helper.py

@@ -10,6 +10,7 @@ from functools import partial, wraps
 import traceback
 
 from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.common.dict_transformations import dict_merge
 
 
 class ModuleHelperException(Exception):
@@ -24,12 +25,12 @@ class ModuleHelperException(Exception):
     def __init__(self, *args, **kwargs):
         self.msg = self._get_remove('msg', kwargs) or "Module failed with exception: {0}".format(self)
         self.update_output = self._get_remove('update_output', kwargs) or {}
-        super(ModuleHelperException, self).__init__(*args, **kwargs)
+        super(ModuleHelperException, self).__init__(*args)
 
 
 class ArgFormat(object):
     """
-    Argument formatter
+    Argument formatter for use as a command line parameter. Used in CmdMixin.
     """
     BOOLEAN = 0
     PRINTF = 1
@@ -50,7 +51,8 @@ class ArgFormat(object):
 
     def __init__(self, name, fmt=None, style=FORMAT, stars=0):
         """
-        Creates a new formatter
+        Creates a CLI-formatter for one specific argument. The argument may be a module parameter or just a named parameter for
+        the CLI command execution.
         :param name: Name of the argument to be formatted
         :param fmt: Either a str to be formatted (using or not printf-style) or a callable that does that
         :param style: Whether arg_format (as str) should use printf-style formatting.
@@ -106,7 +108,7 @@ def cause_changes(func, on_success=True, on_failure=False):
             func(*args, **kwargs)
             if on_success:
                 self.changed = True
-        except Exception as e:
+        except Exception:
             if on_failure:
                 self.changed = True
             raise
@@ -123,11 +125,12 @@ def module_fails_on_exception(func):
         except ModuleHelperException as e:
             if e.update_output:
                 self.update_output(e.update_output)
-            self.module.fail_json(changed=False, msg=e.msg, exception=traceback.format_exc(), output=self.output, vars=self.vars)
+            self.module.fail_json(msg=e.msg, exception=traceback.format_exc(),
+                                  output=self.output, vars=self.vars.output(), **self.output)
         except Exception as e:
-            self.vars.msg = "Module failed with exception: {0}".format(str(e).strip())
+            msg = "Module failed with exception: {0}".format(str(e).strip())
-            self.vars.exception = traceback.format_exc()
+            self.module.fail_json(msg=msg, exception=traceback.format_exc(),
-            self.module.fail_json(changed=False, msg=self.vars.msg, exception=self.vars.exception, output=self.output, vars=self.vars)
+                                  output=self.output, vars=self.vars.output(), **self.output)
     return wrapper
 
 
@@ -141,7 +144,7 @@ class DependencyCtxMgr(object):
         self.exc_tb = None
 
     def __enter__(self):
-        pass
+        return self
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         self.has_it = exc_type is None
@@ -155,17 +158,120 @@ class DependencyCtxMgr(object):
         return self.msg or str(self.exc_val)
 
 
-class ModuleHelper(object):
+class VarMeta(object):
-    _dependencies = []
+    def __init__(self, diff=False, output=False, change=None):
-    module = {}
+        self.init = False
-    facts_name = None
+        self.initial_value = None
+        self.value = None
+
+        self.diff = diff
+        self.change = diff if change is None else change
+        self.output = output
+
+    def set(self, diff=None, output=None, change=None):
+        if diff is not None:
+            self.diff = diff
+        if output is not None:
+            self.output = output
+        if change is not None:
+            self.change = change
+
+    def set_value(self, value):
+        if not self.init:
+            self.initial_value = value
+            self.init = True
+        self.value = value
+        return self
+
+    @property
+    def has_changed(self):
+        return self.change and (self.initial_value != self.value)
+
+    @property
+    def diff_result(self):
+        return None if not (self.diff and self.has_changed) else {
+            'before': self.initial_value,
+            'after': self.value,
+        }
+
+    def __str__(self):
+        return "<VarMeta: value={0}, initial={1}, diff={2}, output={3}, change={4}>".format(
+            self.value, self.initial_value, self.diff, self.output, self.change
+        )
+
+
+class ModuleHelper(object):
+    _output_conflict_list = ('msg', 'exception', 'output', 'vars', 'changed')
+    _dependencies = []
+    module = None
+    facts_name = None
+    output_params = ()
+    diff_params = ()
+    change_params = ()
+
+    class VarDict(object):
+        def __init__(self):
+            self._data = dict()
+            self._meta = dict()
+
+        def __getitem__(self, item):
+            return self._data[item]
+
+        def __setitem__(self, key, value):
+            self.set(key, value)
 
-    class AttrDict(dict):
         def __getattr__(self, item):
-            return self[item]
+            try:
+                return self._data[item]
+            except KeyError:
+                return getattr(self._data, item)
+
+        def __setattr__(self, key, value):
+            if key in ('_data', '_meta'):
+                super(ModuleHelper.VarDict, self).__setattr__(key, value)
+            else:
+                self.set(key, value)
+
+        def meta(self, name):
+            return self._meta[name]
+
+        def set_meta(self, name, **kwargs):
+            self.meta(name).set(**kwargs)
+
+        def set(self, name, value, **kwargs):
+            if name in ('_data', '_meta'):
+                raise ValueError("Names _data and _meta are reserved for use by ModuleHelper")
+            self._data[name] = value
+            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
+
+        def output(self):
+            return dict((k, v) for k, v in self._data.items() if self.meta(k).output)
+
+        def diff(self):
+            diff_results = [(k, self.meta(k).diff_result) for k in self._data]
+            diff_results = [dr for dr in diff_results if dr[1] is not None]
+            if diff_results:
+                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 change_vars(self):
+            return [v for v in self._data if self.meta(v).change]
+
+        def has_changed(self, v):
+            return self._meta[v].has_changed
 
     def __init__(self, module=None):
-        self.vars = ModuleHelper.AttrDict()
+        self.vars = ModuleHelper.VarDict()
         self.output_dict = dict()
         self.facts_dict = dict()
         self._changed = False
@@ -173,9 +279,17 @@ class ModuleHelper(object):
         if module:
             self.module = module
 
-        if isinstance(self.module, dict):
+        if not isinstance(self.module, AnsibleModule):
             self.module = AnsibleModule(**self.module)
 
+        for name, value in self.module.params.items():
+            self.vars.set(
+                name, value,
+                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,
+            )
+
     def update_output(self, **kwargs):
         self.output_dict.update(kwargs)
 
@@ -191,6 +305,9 @@ class ModuleHelper(object):
     def __quit_module__(self):
         pass
 
+    def _vars_changed(self):
+        return any(self.vars.has_changed(v) for v in self.vars.change_vars())
+
     @property
     def changed(self):
         return self._changed
@@ -199,12 +316,24 @@ class ModuleHelper(object):
     def changed(self, value):
         self._changed = value
 
+    def has_changed(self):
+        return self.changed or self._vars_changed()
+
     @property
     def output(self):
-        result = dict(self.vars)
+        result = dict(self.vars.output())
         result.update(self.output_dict)
         if self.facts_name:
             result['ansible_facts'] = {self.facts_name: self.facts_dict}
+        if self.module._diff:
+            diff = result.get('diff', {})
+            vars_diff = self.vars.diff() or {}
+            result['diff'] = dict_merge(dict(diff), vars_diff)
+
+        for varname in result:
+            if varname in self._output_conflict_list:
+                result["_" + varname] = result[varname]
+                del result[varname]
         return result
 
     @module_fails_on_exception
@@ -213,7 +342,7 @@ class ModuleHelper(object):
         self.__init_module__()
         self.__run__()
         self.__quit_module__()
-        self.module.exit_json(changed=self.changed, **self.output_dict)
+        self.module.exit_json(changed=self.has_changed(), **self.output)
 
     @classmethod
     def dependency(cls, name, msg):
@@ -224,9 +353,9 @@ class ModuleHelper(object):
         for d in self._dependencies:
             if not d.has_it:
                 self.module.fail_json(changed=False,
-                                      exception=d.exc_val.__traceback__.format_exc(),
+                                      exception="\n".join(traceback.format_exception(d.exc_type, d.exc_val, d.exc_tb)),
                                       msg=d.text,
-                                      **self.output_dict)
+                                      **self.output)
 
 
 class StateMixin(object):
@@ -332,7 +461,7 @@ class CmdMixin(object):
         return rc, out, err
 
     def run_command(self, extra_params=None, params=None, *args, **kwargs):
-        self.vars['cmd_args'] = self._calculate_args(extra_params, params)
+        self.vars.cmd_args = self._calculate_args(extra_params, params)
         options = dict(self.run_command_fixed_options)
         env_update = dict(options.get('environ_update', {}))
         options['check_rc'] = options.get('check_rc', self.check_rc)
@@ -341,7 +470,7 @@ class CmdMixin(object):
             self.update_output(force_lang=self.force_lang)
             options['environ_update'] = env_update
         options.update(kwargs)
-        rc, out, err = self.module.run_command(self.vars['cmd_args'], *args, **options)
+        rc, out, err = self.module.run_command(self.vars.cmd_args, *args, **options)
         self.update_output(rc=rc, stdout=out, stderr=err)
         return self.process_command_output(rc, out, err)
 

tests/integration/targets/module_helper/aliases

@@ -0,0 +1 @@
+shippable/posix/group4

tests/integration/targets/module_helper/library/mdepfail.py

@@ -0,0 +1,69 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# (c) 2021, Alexei Znamensky <russoz@gmail.com>
+#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+DOCUMENTATION = '''
+module: mdepfail
+author: "Alexei Znamensky (@russoz)"
+short_description: Simple module for testing
+description:
+  - Simple module test description.
+options:
+  a:
+    description: aaaa
+    type: int
+  b:
+    description: bbbb
+    type: str
+  c:
+    description: cccc
+    type: str
+'''
+
+EXAMPLES = ""
+
+RETURN = ""
+
+from ansible_collections.community.general.plugins.module_utils.module_helper import ModuleHelper
+from ansible.module_utils.basic import missing_required_lib
+
+with ModuleHelper.dependency("nopackagewiththisname", missing_required_lib("nopackagewiththisname")):
+    import nopackagewiththisname
+
+
+class MSimple(ModuleHelper):
+    output_params = ('a', 'b', 'c')
+    module = dict(
+        argument_spec=dict(
+            a=dict(type='int'),
+            b=dict(type='str'),
+            c=dict(type='str'),
+        ),
+    )
+
+    def __init_module__(self):
+        self.vars.set('value', None)
+        self.vars.set('abc', "abc", diff=True)
+
+    def __run__(self):
+        if (0 if self.vars.a is None else self.vars.a) >= 100:
+            raise Exception("a >= 100")
+        if self.vars.c == "abc change":
+            self.vars['abc'] = "changed abc"
+        if self.vars.get('a', 0) == 2:
+            self.vars['b'] = str(self.vars.b) * 2
+            self.vars['c'] = str(self.vars.c) * 2
+
+
+def main():
+    msimple = MSimple()
+    msimple.run()
+
+
+if __name__ == '__main__':
+    main()

tests/integration/targets/module_helper/library/msimple.py

@@ -0,0 +1,65 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# (c) 2021, Alexei Znamensky <russoz@gmail.com>
+#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+DOCUMENTATION = '''
+module: msimple
+author: "Alexei Znamensky (@russoz)"
+short_description: Simple module for testing
+description:
+  - Simple module test description.
+options:
+  a:
+    description: aaaa
+    type: int
+  b:
+    description: bbbb
+    type: str
+  c:
+    description: cccc
+    type: str
+'''
+
+EXAMPLES = ""
+
+RETURN = ""
+
+from ansible_collections.community.general.plugins.module_utils.module_helper import ModuleHelper
+
+
+class MSimple(ModuleHelper):
+    output_params = ('a', 'b', 'c')
+    module = dict(
+        argument_spec=dict(
+            a=dict(type='int'),
+            b=dict(type='str'),
+            c=dict(type='str'),
+        ),
+    )
+
+    def __init_module__(self):
+        self.vars.set('value', None)
+        self.vars.set('abc', "abc", diff=True)
+
+    def __run__(self):
+        if (0 if self.vars.a is None else self.vars.a) >= 100:
+            raise Exception("a >= 100")
+        if self.vars.c == "abc change":
+            self.vars['abc'] = "changed abc"
+        if self.vars.get('a', 0) == 2:
+            self.vars['b'] = str(self.vars.b) * 2
+            self.vars['c'] = str(self.vars.c) * 2
+
+
+def main():
+    msimple = MSimple()
+    msimple.run()
+
+
+if __name__ == '__main__':
+    main()

tests/integration/targets/module_helper/library/mstate.py

@@ -0,0 +1,77 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# (c) 2021, Alexei Znamensky <russoz@gmail.com>
+#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+DOCUMENTATION = '''
+module: mstate
+author: "Alexei Znamensky (@russoz)"
+short_description: State-based module for testing
+description:
+  - State-based module test description.
+options:
+  a:
+    description: aaaa
+    type: int
+    required: yes
+  b:
+    description: bbbb
+    type: str
+  c:
+    description: cccc
+    type: str
+  state:
+    description: test states
+    type: str
+    choices: [join, b_x_a, c_x_a, both_x_a]
+    default: join
+'''
+
+EXAMPLES = ""
+
+RETURN = ""
+
+from ansible_collections.community.general.plugins.module_utils.module_helper import StateModuleHelper
+
+
+class MState(StateModuleHelper):
+    output_params = ('a', 'b', 'c', 'state')
+    module = dict(
+        argument_spec=dict(
+            a=dict(type='int', required=True),
+            b=dict(type='str'),
+            c=dict(type='str'),
+            state=dict(type='str', choices=['join', 'b_x_a', 'c_x_a', 'both_x_a', 'nop'], default='join'),
+        ),
+    )
+
+    def __init_module__(self):
+        self.vars.set('result', "abc", diff=True)
+
+    def state_join(self):
+        self.vars['result'] = "".join([str(self.vars.a), str(self.vars.b), str(self.vars.c)])
+
+    def state_b_x_a(self):
+        self.vars['result'] = str(self.vars.b) * self.vars.a
+
+    def state_c_x_a(self):
+        self.vars['result'] = str(self.vars.c) * self.vars.a
+
+    def state_both_x_a(self):
+        self.vars['result'] = (str(self.vars.b) + str(self.vars.c)) * self.vars.a
+
+    def state_nop(self):
+        pass
+
+
+def main():
+    mstate = MState()
+    mstate.run()
+
+
+if __name__ == '__main__':
+    main()

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

@@ -0,0 +1,3 @@
+- include_tasks: msimple.yml
+- include_tasks: mdepfail.yml
+- include_tasks: mstate.yml

tests/integration/targets/module_helper/tasks/mdepfail.yml

@@ -0,0 +1,14 @@
+- name: test failing dependency
+  mdepfail:
+    a: 123
+  ignore_errors: yes
+  register: result
+
+- name: assert failing dependency
+  assert:
+    that:
+      - result.failed is true
+      - '"Failed to import" in result.msg'
+      - '"nopackagewiththisname" in result.msg'
+      - '"ModuleNotFoundError:" in result.exception'
+      - '"nopackagewiththisname" in result.exception'

tests/integration/targets/module_helper/tasks/msimple.yml

@@ -0,0 +1,54 @@
+- name: test msimple 1
+  msimple:
+    a: 80
+  register: simple1
+
+- name: assert simple1
+  assert:
+    that:
+      - simple1.a == 80
+      - simple1.abc == "abc"
+      - simple1.changed is false
+      - simple1.value is none
+
+- name: test msimple 2
+  msimple:
+    a: 101
+  ignore_errors: yes
+  register: simple2
+
+- name: assert simple2
+  assert:
+    that:
+      - simple2.a == 101
+      - 'simple2.msg == "Module failed with exception: a >= 100"'
+      - simple2.abc == "abc"
+      - simple2.failed is true
+      - simple2.changed is false
+      - simple2.value is none
+
+- name: test msimple 3
+  msimple:
+    a: 2
+    b: potatoes
+  register: simple3
+
+- name: assert simple3
+  assert:
+    that:
+      - simple3.a == 2
+      - simple3.b == "potatoespotatoes"
+      - simple3.c == "NoneNone"
+      - simple3.changed is false
+
+- name: test msimple 4
+  msimple:
+    c: abc change
+  register: simple4
+
+- name: assert simple4
+  assert:
+    that:
+      - simple4.c == "abc change"
+      - simple4.abc == "changed abc"
+      - simple4.changed is true

tests/integration/targets/module_helper/tasks/mstate.yml

@@ -0,0 +1,79 @@
+- name: test mstate 1
+  mstate:
+    a: 80
+    b: banana
+    c: cashew
+    state: nop
+  register: state1
+
+- name: assert state1
+  assert:
+    that:
+      - state1.a == 80
+      - state1.b == "banana"
+      - state1.c == "cashew"
+      - state1.result == "abc"
+      - state1.changed is false
+
+- name: test mstate 2
+  mstate:
+    a: 80
+    b: banana
+    c: cashew
+  register: state2
+
+- name: assert state2
+  assert:
+    that:
+      - state2.a == 80
+      - state2.b == "banana"
+      - state2.c == "cashew"
+      - state2.result == "80bananacashew"
+      - state2.changed is true
+
+- name: test mstate 3
+  mstate:
+    a: 3
+    b: banana
+    state: b_x_a
+  register: state3
+
+- name: assert state3
+  assert:
+    that:
+      - state3.a == 3
+      - state3.b == "banana"
+      - state3.result == "bananabananabanana"
+      - state3.changed is true
+
+- name: test mstate 4
+  mstate:
+    a: 4
+    c: cashew
+    state: c_x_a
+  register: state4
+
+- name: assert state4
+  assert:
+    that:
+      - state4.a == 4
+      - state4.c == "cashew"
+      - state4.result == "cashewcashewcashewcashew"
+      - state4.changed is true
+
+- name: test mstate 5
+  mstate:
+    a: 5
+    b: foo
+    c: bar
+    state: both_x_a
+  register: state5
+
+- name: assert state5
+  assert:
+    that:
+      - state5.a == 5
+      - state5.b == "foo"
+      - state5.c == "bar"
+      - state5.result == "foobarfoobarfoobarfoobarfoobar"
+      - state5.changed is true

tests/unit/plugins/module_utils/test_module_helper.py

@@ -9,7 +9,7 @@ __metaclass__ = type
 import pytest
 
 from ansible_collections.community.general.plugins.module_utils.module_helper import (
-    ArgFormat, DependencyCtxMgr, ModuleHelper
+    ArgFormat, DependencyCtxMgr, ModuleHelper, VarMeta
 )
 
 
@@ -105,3 +105,58 @@ def test_dependency_ctxmgr():
     with ctx:
         import sys
     assert ctx.has_it
+
+
+def test_variable_meta():
+    meta = VarMeta()
+    assert meta.output is False
+    assert meta.diff is False
+    assert meta.value is None
+    meta.set_value("abc")
+    assert meta.initial_value == "abc"
+    assert meta.value == "abc"
+    assert meta.diff_result is None
+    meta.set_value("def")
+    assert meta.initial_value == "abc"
+    assert meta.value == "def"
+    assert meta.diff_result is None
+
+
+def test_variable_meta_diff():
+    meta = VarMeta(diff=True)
+    assert meta.output is False
+    assert meta.diff is True
+    assert meta.value is None
+    meta.set_value("abc")
+    assert meta.initial_value == "abc"
+    assert meta.value == "abc"
+    assert meta.diff_result is None
+    meta.set_value("def")
+    assert meta.initial_value == "abc"
+    assert meta.value == "def"
+    assert meta.diff_result == {"before": "abc", "after": "def"}
+    meta.set_value("ghi")
+    assert meta.initial_value == "abc"
+    assert meta.value == "ghi"
+    assert meta.diff_result == {"before": "abc", "after": "ghi"}
+
+
+def test_vardict():
+    vd = ModuleHelper.VarDict()
+    vd.set('a', 123)
+    assert vd['a'] == 123
+    assert vd.a == 123
+    assert 'a' in vd._meta
+    assert vd.meta('a').output is True
+    assert vd.meta('a').diff is False
+    assert vd.meta('a').change is False
+    vd['b'] = 456
+    vd.set_meta('a', diff=True, change=True)
+    vd.set_meta('b', diff=True, output=False)
+    vd['c'] = 789
+    vd['a'] = 'new_a'
+    vd['c'] = 'new_c'
+    assert vd.a == 'new_a'
+    assert vd.c == 'new_c'
+    assert vd.output() == {'a': 'new_a', 'c': 'new_c'}
+    assert vd.diff() == {'before': {'a': 123}, 'after': {'a': 'new_a'}}, "diff={0}".format(vd.diff())