From 88d0e2a04a4d353b31a4cf561987084f08a9a573 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Thu, 21 Jun 2018 16:14:57 -0400 Subject: [PATCH] fix minor issues with debug and item labels (#41331) * fix minor issues with debug and item labels - no more `item=None`, we always have a label now - debug should only show expected information, either msg= or the var in var= - also fixed method name, deprecated misleading _get_item (cherry picked from commit 27c43daab861190f1778824c2712ebe051b3352b) --- changelogs/fragments/debug_fixes.yml | 2 ++ lib/ansible/executor/task_executor.py | 17 +++++---- lib/ansible/plugins/callback/__init__.py | 38 +++++++++++++------- lib/ansible/plugins/callback/default.py | 6 ++-- test/units/plugins/callback/test_callback.py | 17 +++++++++ 5 files changed, 59 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/debug_fixes.yml diff --git a/changelogs/fragments/debug_fixes.yml b/changelogs/fragments/debug_fixes.yml new file mode 100644 index 0000000000..ced8bbff61 --- /dev/null +++ b/changelogs/fragments/debug_fixes.yml @@ -0,0 +1,2 @@ +bugfixes: + - correct debug display for all cases https://github.com/ansible/ansible/pull/41331 diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 7e74de440e..87999f2a58 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -278,14 +278,19 @@ class TaskExecutor: label = None loop_pause = 0 templar = Templar(loader=self._loader, shared_loader_obj=self._shared_loader_obj, variables=self._job_vars) + + # FIXME: move this to the object itself to allow post_validate to take care of templating (loop_control.post_validate) if self._task.loop_control: - # FIXME: move this to the object itself to allow post_validate to take care of templating loop_var = templar.template(self._task.loop_control.loop_var) index_var = templar.template(self._task.loop_control.index_var) loop_pause = templar.template(self._task.loop_control.pause) - # the these may be 'None', so we still need to default to something useful - # this is tempalted below after an item is assigned - label = (self._task.loop_control.label or ('{{' + loop_var + '}}')) + + # This may be 'None',so it is tempalted below after we ensure a value and an item is assigned + label = self._task.loop_control.label + + # ensure we always have a label + if label is None: + label = '{{' + loop_var + '}}' if loop_var in task_vars: display.warning(u"The loop variable '%s' is already in use. " @@ -339,8 +344,8 @@ class TaskExecutor: res['_ansible_item_result'] = True res['_ansible_ignore_errors'] = task_fields.get('ignore_errors') - if label is not None: - res['_ansible_item_label'] = templar.template(label, cache=False) + # gets templated here unlike rest of loop_control fields, depends on loop_var above + res['_ansible_item_label'] = templar.template(label, cache=False) self._rslt_q.put( TaskResult( diff --git a/lib/ansible/plugins/callback/__init__.py b/lib/ansible/plugins/callback/__init__.py index 23db65629d..2a56911b6b 100644 --- a/lib/ansible/plugins/callback/__init__.py +++ b/lib/ansible/plugins/callback/__init__.py @@ -21,9 +21,12 @@ __metaclass__ = type import difflib import json +import os import sys import warnings + from copy import deepcopy +from collections import MutableMapping from ansible import constants as C from ansible.parsing.ajson import AnsibleJSONEncoder @@ -78,7 +81,7 @@ class CallbackBase(AnsiblePlugin): if options is not None: self.set_options(options) - self._hide_in_debug = ('changed', 'failed', 'skipped', 'invocation') + self._hide_in_debug = ('changed', 'failed', 'skipped', 'invocation', 'skip_reason') ''' helper for callbacks, so they don't all have to include deepcopy ''' _copy_result = deepcopy @@ -172,7 +175,7 @@ class CallbackBase(AnsiblePlugin): if 'before' in diff and 'after' in diff: # format complex structures into 'files' for x in ['before', 'after']: - if isinstance(diff[x], dict): + if isinstance(diff[x], MutableMapping): diff[x] = json.dumps(diff[x], sort_keys=True, indent=4, separators=(',', ': ')) + '\n' if 'before_header' in diff: before_header = "before: %s" % diff['before_header'] @@ -221,27 +224,38 @@ class CallbackBase(AnsiblePlugin): ret.append(">> the files are different, but the diff library cannot compare unicode strings\n\n") return u''.join(ret) - def _get_item(self, result): + def _get_item_label(self, result): + ''' retrieves the value to be displayed as a label for an item entry from a result object''' if result.get('_ansible_no_log', False): item = "(censored due to no_log)" - elif result.get('_ansible_item_label', False): - item = result.get('_ansible_item_label') else: - item = result.get('item', None) - + item = result.get('_ansible_item_label', result.get('item')) return item + def _get_item(self, result): + ''' here for backwards compat, really should have always been named: _get_item_label''' + cback = getattr(self, 'NAME', os.path.basename(__file__)) + self._display.deprecated("The %s callback plugin should be updated to use the _get_item_label method instead" % cback, version="2.11") + return self._get_item_label(result) + def _process_items(self, result): # just remove them as now they get handled by individual callbacks del result._result['results'] def _clean_results(self, result, task_name): ''' removes data from results for display ''' + + # mostly controls that debug only outputs what it was meant to if task_name in ['debug']: - for hideme in self._hide_in_debug: - result.pop(hideme, None) - if 'msg' in result: - result.pop('item', None) + if 'msg' in result: + # msg should be alone + for key in list(result.keys()): + if key != 'msg' and not key.startswith('_'): + result.pop(key) + else: + # 'var' value as field, so eliminate others and what is left should be varname + for hidme in self._hide_in_debug: + result.pop(hidme, None) def set_play_context(self, play_context): pass @@ -324,7 +338,7 @@ class CallbackBase(AnsiblePlugin): def v2_runner_on_skipped(self, result): if C.DISPLAY_SKIPPED_HOSTS: host = result._host.get_name() - self.runner_on_skipped(host, self._get_item(getattr(result._result, 'results', {}))) + self.runner_on_skipped(host, self._get_item_label(getattr(result._result, 'results', {}))) def v2_runner_on_unreachable(self, result): host = result._host.get_name() diff --git a/lib/ansible/plugins/callback/default.py b/lib/ansible/plugins/callback/default.py index c38e1c58a4..763039e9ea 100644 --- a/lib/ansible/plugins/callback/default.py +++ b/lib/ansible/plugins/callback/default.py @@ -205,7 +205,7 @@ class CallbackModule(CallbackBase): else: msg += ": [%s]" % result._host.get_name() - msg += " => (item=%s)" % (self._get_item(result._result),) + msg += " => (item=%s)" % (self._get_item_label(result._result),) if (self._display.verbosity > 0 or '_ansible_verbose_always' in result._result) and '_ansible_verbose_override' not in result._result: msg += " => %s" % self._dump_results(result._result) @@ -224,12 +224,12 @@ class CallbackModule(CallbackBase): msg += "[%s]" % (result._host.get_name()) self._handle_warnings(result._result) - self._display.display(msg + " (item=%s) => %s" % (self._get_item(result._result), self._dump_results(result._result)), color=C.COLOR_ERROR) + self._display.display(msg + " (item=%s) => %s" % (self._get_item_label(result._result), self._dump_results(result._result)), color=C.COLOR_ERROR) def v2_runner_item_on_skipped(self, result): if self._plugin_options.get('show_skipped_hosts', C.DISPLAY_SKIPPED_HOSTS): # fallback on constants for inherited plugins missing docs self._clean_results(result._result, result._task.action) - msg = "skipping: [%s] => (item=%s) " % (result._host.get_name(), self._get_item(result._result)) + msg = "skipping: [%s] => (item=%s) " % (result._host.get_name(), self._get_item_label(result._result)) if (self._display.verbosity > 0 or '_ansible_verbose_always' in result._result) and '_ansible_verbose_override' not in result._result: msg += " => %s" % self._dump_results(result._result) self._display.display(msg, color=C.COLOR_SKIP) diff --git a/test/units/plugins/callback/test_callback.py b/test/units/plugins/callback/test_callback.py index 86fbe508e7..1df4b55d7d 100644 --- a/test/units/plugins/callback/test_callback.py +++ b/test/units/plugins/callback/test_callback.py @@ -51,6 +51,7 @@ class TestCallback(unittest.TestCase): class TestCallbackResults(unittest.TestCase): + def test_get_item(self): cb = CallbackBase() results = {'item': 'some_item'} @@ -67,6 +68,22 @@ class TestCallbackResults(unittest.TestCase): res = cb._get_item(results) self.assertEquals(res, "some_item") + def test_get_item_label(self): + cb = CallbackBase() + results = {'item': 'some_item'} + res = cb._get_item_label(results) + self.assertEquals(res, 'some_item') + + def test_get_item_label_no_log(self): + cb = CallbackBase() + results = {'item': 'some_item', '_ansible_no_log': True} + res = cb._get_item_label(results) + self.assertEquals(res, "(censored due to no_log)") + + results = {'item': 'some_item', '_ansible_no_log': False} + res = cb._get_item_label(results) + self.assertEquals(res, "some_item") + def test_clean_results_debug_task(self): cb = CallbackBase() result = {'item': 'some_item',