From 1422ffeffdae4938d090c0e709a4f8764488169f Mon Sep 17 00:00:00 2001 From: Alexei Znamensky Date: Thu, 5 Jan 2023 16:33:36 +1300 Subject: [PATCH 1/5] ModuleHelper - lax handling of conflicting output --- plugins/module_utils/mh/deco.py | 8 +++++-- plugins/module_utils/mh/module_helper.py | 1 - .../targets/module_helper/library/msimple.py | 5 ++-- .../tasks/msimple_output_conflict.yml | 23 +++++++++++++++++-- 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/plugins/module_utils/mh/deco.py b/plugins/module_utils/mh/deco.py index 3073e4e9e71..3018678a349 100644 --- a/plugins/module_utils/mh/deco.py +++ b/plugins/module_utils/mh/deco.py @@ -46,12 +46,16 @@ def wrapper(self, *args, **kwargs): except ModuleHelperException as e: if e.update_output: self.update_output(e.update_output) + # patchy solution to resolve conflict with output variables + output = dict([(k, v) for k, v in self.output.items() if k not in self._output_conflict_list]) self.module.fail_json(msg=e.msg, exception=traceback.format_exc(), - output=self.output, vars=self.vars.output(), **self.output) + output=self.output, vars=self.vars.output(), **output) except Exception as e: + # patchy solution to resolve conflict with output variables + output = dict([(k, v) for k, v in self.output.items() if k not in self._output_conflict_list]) msg = "Module failed with exception: {0}".format(str(e).strip()) self.module.fail_json(msg=msg, exception=traceback.format_exc(), - output=self.output, vars=self.vars.output(), **self.output) + output=self.output, vars=self.vars.output(), **output) return wrapper diff --git a/plugins/module_utils/mh/module_helper.py b/plugins/module_utils/mh/module_helper.py index 51696b6ff6e..795271348a2 100644 --- a/plugins/module_utils/mh/module_helper.py +++ b/plugins/module_utils/mh/module_helper.py @@ -63,7 +63,6 @@ def output(self): for varname in list(result): if varname in self._output_conflict_list: result["_" + varname] = result[varname] - del result[varname] return result diff --git a/tests/integration/targets/module_helper/library/msimple.py b/tests/integration/targets/module_helper/library/msimple.py index 3729b6c7028..096e515247b 100644 --- a/tests/integration/targets/module_helper/library/msimple.py +++ b/tests/integration/targets/module_helper/library/msimple.py @@ -57,6 +57,8 @@ def process_a3_bc(self): self.vars['c'] = str(self.vars.c) * 3 def __run__(self): + if self.vars.m: + self.vars.msg = self.vars.m if self.vars.a >= 100: raise Exception("a >= 100") if self.vars.c == "abc change": @@ -66,9 +68,6 @@ def __run__(self): self.vars['c'] = str(self.vars.c) * 2 self.process_a3_bc() - if self.vars.m: - self.vars.msg = self.vars.m - def main(): msimple = MSimple() diff --git a/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml b/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml index 21ffd37d350..4a8ed25399c 100644 --- a/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml +++ b/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml @@ -2,7 +2,7 @@ # 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 -- name: test msimple (set a=80) +- name: test msimple conflict output (set a=80) msimple: a: 80 register: simple1 @@ -15,7 +15,7 @@ - simple1 is not changed - simple1.value is none -- name: test msimple 2 +- name: test msimple conflict output 2 msimple: a: 80 m: a message in a bottle @@ -29,3 +29,22 @@ - simple1 is not changed - simple1.value is none - 'simple2._msg == "a message in a bottle"' + - 'simple2.msg == "a message in a bottle"' + +- name: test msimple 3 + msimple: + a: 101 + m: a message in a bottle + ignore_errors: yes + register: simple3 + +- name: assert simple3 + assert: + that: + - simple3.a == 101 + - 'simple3.msg == "Module failed with exception: a >= 100"' + - 'simple3._msg == "a message in a bottle"' + - simple3.abc == "abc" + - simple3 is failed + - simple3 is not changed + - simple3.value is none From eab7a5bd96ff7182d033c18d89f418538117fd55 Mon Sep 17 00:00:00 2001 From: Alexei Znamensky Date: Thu, 5 Jan 2023 16:41:11 +1300 Subject: [PATCH 2/5] add changelog fragment --- changelogs/fragments/5765-mh-lax-output-conflict.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/5765-mh-lax-output-conflict.yml diff --git a/changelogs/fragments/5765-mh-lax-output-conflict.yml b/changelogs/fragments/5765-mh-lax-output-conflict.yml new file mode 100644 index 00000000000..30ed4be1a21 --- /dev/null +++ b/changelogs/fragments/5765-mh-lax-output-conflict.yml @@ -0,0 +1,2 @@ +minor_changes: + - ModuleHelper module utils - handling of output variables with protected names now allows module to set them if unused by MH itself (https://github.com/ansible-collections/community.general/pull/5765). From 2ba9d6d7230c5b47d8aa5bee1f0e9a9b04c7a263 Mon Sep 17 00:00:00 2001 From: Alexei Znamensky Date: Thu, 5 Jan 2023 23:00:41 +1300 Subject: [PATCH 3/5] only create _var when really needed --- plugins/module_utils/mh/deco.py | 13 +++++++++++-- plugins/module_utils/mh/module_helper.py | 4 ---- .../module_helper/tasks/msimple_output_conflict.yml | 12 ++++++++---- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/plugins/module_utils/mh/deco.py b/plugins/module_utils/mh/deco.py index 3018678a349..5138b212c7f 100644 --- a/plugins/module_utils/mh/deco.py +++ b/plugins/module_utils/mh/deco.py @@ -37,8 +37,17 @@ def wrapper(*args, **kwargs): def module_fails_on_exception(func): + conflict_list = ('msg', 'exception', 'output', 'vars', 'changed') + @wraps(func) def wrapper(self, *args, **kwargs): + def fix_var_conflicts(output): + result = dict([ + (k if k not in conflict_list else "_" + k, v) + for k, v in output.items() + ]) + return result + try: func(self, *args, **kwargs) except SystemExit: @@ -47,12 +56,12 @@ def wrapper(self, *args, **kwargs): if e.update_output: self.update_output(e.update_output) # patchy solution to resolve conflict with output variables - output = dict([(k, v) for k, v in self.output.items() if k not in self._output_conflict_list]) + output = fix_var_conflicts(self.output) self.module.fail_json(msg=e.msg, exception=traceback.format_exc(), output=self.output, vars=self.vars.output(), **output) except Exception as e: # patchy solution to resolve conflict with output variables - output = dict([(k, v) for k, v in self.output.items() if k not in self._output_conflict_list]) + output = fix_var_conflicts(self.output) msg = "Module failed with exception: {0}".format(str(e).strip()) self.module.fail_json(msg=msg, exception=traceback.format_exc(), output=self.output, vars=self.vars.output(), **output) diff --git a/plugins/module_utils/mh/module_helper.py b/plugins/module_utils/mh/module_helper.py index 795271348a2..6813b5454b6 100644 --- a/plugins/module_utils/mh/module_helper.py +++ b/plugins/module_utils/mh/module_helper.py @@ -18,7 +18,6 @@ class ModuleHelper(DeprecateAttrsMixin, VarsMixin, DependencyMixin, ModuleHelperBase): - _output_conflict_list = ('msg', 'exception', 'output', 'vars', 'changed') facts_name = None output_params = () diff_params = () @@ -60,9 +59,6 @@ def output(self): vars_diff = self.vars.diff() or {} result['diff'] = dict_merge(dict(diff), vars_diff) - for varname in list(result): - if varname in self._output_conflict_list: - result["_" + varname] = result[varname] return result diff --git a/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml b/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml index 4a8ed25399c..55e0a06eca4 100644 --- a/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml +++ b/tests/integration/targets/module_helper/tasks/msimple_output_conflict.yml @@ -28,8 +28,10 @@ - simple1.abc == "abc" - simple1 is not changed - simple1.value is none - - 'simple2._msg == "a message in a bottle"' - - 'simple2.msg == "a message in a bottle"' + - > + "_msg" not in simple2 + - > + simple2.msg == "a message in a bottle" - name: test msimple 3 msimple: @@ -42,8 +44,10 @@ assert: that: - simple3.a == 101 - - 'simple3.msg == "Module failed with exception: a >= 100"' - - 'simple3._msg == "a message in a bottle"' + - > + simple3.msg == "Module failed with exception: a >= 100" + - > + simple3._msg == "a message in a bottle" - simple3.abc == "abc" - simple3 is failed - simple3 is not changed From 174c15ce509107ef35b297f91f386419b7c084a8 Mon Sep 17 00:00:00 2001 From: Alexei Znamensky Date: Fri, 6 Jan 2023 12:52:32 +1300 Subject: [PATCH 4/5] adjust changelog --- changelogs/fragments/5765-mh-lax-output-conflict.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/changelogs/fragments/5765-mh-lax-output-conflict.yml b/changelogs/fragments/5765-mh-lax-output-conflict.yml index 30ed4be1a21..9f3e9c588ac 100644 --- a/changelogs/fragments/5765-mh-lax-output-conflict.yml +++ b/changelogs/fragments/5765-mh-lax-output-conflict.yml @@ -1,2 +1,5 @@ -minor_changes: - - ModuleHelper module utils - handling of output variables with protected names now allows module to set them if unused by MH itself (https://github.com/ansible-collections/community.general/pull/5765). +breaking_changes: + - > + ModuleHelper module utils - when the module sets output variables named ``msg``, ``exception``, ``output``, ``vars``, or ``changed``, + the actual output will prefix those names with ``_`` (underscore symbol) only when they clash with output variables generated by ModuleHelper + itself, which only occurs when handling exceptions (https://github.com/ansible-collections/community.general/pull/5765). From 5f076fbdd5986f608051b4be55c5bd9dbe5ba285 Mon Sep 17 00:00:00 2001 From: Alexei Znamensky <103110+russoz@users.noreply.github.com> Date: Sat, 7 Jan 2023 12:30:08 +1300 Subject: [PATCH 5/5] Update changelogs/fragments/5765-mh-lax-output-conflict.yml Co-authored-by: Felix Fontein --- changelogs/fragments/5765-mh-lax-output-conflict.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/changelogs/fragments/5765-mh-lax-output-conflict.yml b/changelogs/fragments/5765-mh-lax-output-conflict.yml index 9f3e9c588ac..2e8cc292bd0 100644 --- a/changelogs/fragments/5765-mh-lax-output-conflict.yml +++ b/changelogs/fragments/5765-mh-lax-output-conflict.yml @@ -2,4 +2,8 @@ breaking_changes: - > ModuleHelper module utils - when the module sets output variables named ``msg``, ``exception``, ``output``, ``vars``, or ``changed``, the actual output will prefix those names with ``_`` (underscore symbol) only when they clash with output variables generated by ModuleHelper - itself, which only occurs when handling exceptions (https://github.com/ansible-collections/community.general/pull/5765). + itself, which only occurs when handling exceptions. Please note that this breaking + change does not require a new major release since before this release, it was not possible + to add such variables to the output + `due to a bug `__ + (https://github.com/ansible-collections/community.general/pull/5765).