diff options
author | Brian Coca <bcoca@users.noreply.github.com> | 2020-12-07 18:28:27 -0500 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-12-07 17:28:27 -0600 |
commit | c4844e992c65c16b9e018e2d354afd7011d8d2e1 (patch) | |
tree | 9015bd5b2223e8a2adf4472f0417edfcd84b9b0f | |
parent | c2ca8317b7ccbd689ea29d3d84d07a7d96b2fe64 (diff) | |
download | ansible-c4844e992c65c16b9e018e2d354afd7011d8d2e1.tar.gz |
make collection callbacks follow normal flow (#59932) (#72228)
* make collection callbacks follow normal flow (#59932)
make collections whitelist follow normal flow
* fixes missing set_options call and adhoc and stdout processing rules
* avoid dupes
* fixed to handle redirects
* also updated tests with new and more accurate skip message
* fix callback tests for envs with cowsay installed
* lots MOAR comments on why the code is as it is, some todos to refactor in future
Co-authored-by: Matt Davis <nitzmahone@users.noreply.github.com>
(cherry picked from commit 5ec53f9db853709c99e73d1b9c7b2ae81c681354)
* fixed bad merge
* hack in redirected names
* ensure we run additional calblacks
-rw-r--r-- | changelogs/fragments/collections_cb_fix.yml | 2 | ||||
-rw-r--r-- | lib/ansible/executor/task_queue_manager.py | 66 | ||||
-rwxr-xr-x | test/integration/targets/callback_default/runme.sh | 6 | ||||
-rwxr-xr-x | test/integration/targets/collections/runme.sh | 2 |
4 files changed, 60 insertions, 16 deletions
diff --git a/changelogs/fragments/collections_cb_fix.yml b/changelogs/fragments/collections_cb_fix.yml new file mode 100644 index 0000000000..3b4ac7c193 --- /dev/null +++ b/changelogs/fragments/collections_cb_fix.yml @@ -0,0 +1,2 @@ +bugfixes: + - Collection callbacks were ignoring options and rules for stdout and adhoc cases. diff --git a/lib/ansible/executor/task_queue_manager.py b/lib/ansible/executor/task_queue_manager.py index a71d6c233b..54174aea63 100644 --- a/lib/ansible/executor/task_queue_manager.py +++ b/lib/ansible/executor/task_queue_manager.py @@ -139,35 +139,71 @@ class TaskQueueManager: else: raise AnsibleError("callback must be an instance of CallbackBase or the name of a callback plugin") - for callback_plugin in callback_loader.all(class_only=True): + # get all configured loadable callbacks (adjacent, builtin) + callback_list = list(callback_loader.all(class_only=True)) + + # add whitelisted callbacks that refer to collections, which might not appear in normal listing + for c in C.DEFAULT_CALLBACK_WHITELIST: + # load all, as collection ones might be using short/redirected names and not a fqcn + plugin = callback_loader.get(c, class_only=True) + + # TODO: check if this skip is redundant, loader should handle bad file/plugin cases already + if plugin: + # avoids incorrect and dupes possible due to collections + if plugin not in callback_list: + setattr(plugin, '_redirected_names', [c]) # here for backport as newer versions of plugin_loader already do this + callback_list.append(plugin) + else: + display.warning("Skipping callback plugin '%s', unable to load" % c) + + # for each callback in the list see if we should add it to 'active callbacks' used in the play + for callback_plugin in callback_list: + callback_type = getattr(callback_plugin, 'CALLBACK_TYPE', '') callback_needs_whitelist = getattr(callback_plugin, 'CALLBACK_NEEDS_WHITELIST', False) - (callback_name, _) = os.path.splitext(os.path.basename(callback_plugin._original_path)) + + # try to get colleciotn world name first + cnames = getattr(callback_plugin, '_redirected_names', []) + if cnames: + # store the name the plugin was loaded as, as that's what we'll need to compare to the configured callback list later + callback_name = cnames[0] + else: + # fallback to 'old loader name' + (callback_name, _) = os.path.splitext(os.path.basename(callback_plugin._original_path)) + + display.vvvvv("Attempting to use '%s' callback." % (callback_name)) if callback_type == 'stdout': # we only allow one callback of type 'stdout' to be loaded, if callback_name != self._stdout_callback or stdout_callback_loaded: + display.vv("Skipping callback '%s', as we already have a stdout callback." % (callback_name)) continue stdout_callback_loaded = True elif callback_name == 'tree' and self._run_tree: - # special case for ansible cli option + # TODO: remove special case for tree, which is an adhoc cli option --tree pass elif not self._run_additional_callbacks or (callback_needs_whitelist and ( + # only run if not adhoc, or adhoc was specifically configured to run + check enabled list C.DEFAULT_CALLBACK_WHITELIST is None or callback_name not in C.DEFAULT_CALLBACK_WHITELIST)): # 2.x plugins shipped with ansible should require whitelisting, older or non shipped should load automatically continue - callback_obj = callback_plugin() - callback_obj.set_options() - self._callback_plugins.append(callback_obj) - - for callback_plugin_name in (c for c in C.DEFAULT_CALLBACK_WHITELIST if AnsibleCollectionRef.is_valid_fqcr(c)): - # TODO: need to extend/duplicate the stdout callback check here (and possible move this ahead of the old way - callback_obj = callback_loader.get(callback_plugin_name) - if callback_obj: - callback_obj.set_options() - self._callback_plugins.append(callback_obj) - else: - display.warning("Skipping '%s', unable to load or use as a callback" % callback_plugin_name) + try: + callback_obj = callback_plugin() + # avoid bad plugin not returning an object, only needed cause we do class_only load and bypass loader checks, + # really a bug in the plugin itself which we ignore as callback errors are not supposed to be fatal. + if callback_obj: + # skip initializing if we already did the work for the same plugin (even with diff names) + if callback_obj not in self._callback_plugins: + callback_obj.set_options() + self._callback_plugins.append(callback_obj) + else: + display.vv("Skipping callback '%s', already loaded as '%s'." % (callback_plugin, callback_name)) + else: + display.warning("Skipping callback '%s', as it does not create a valid plugin instance." % callback_name) + continue + except Exception as e: + display.warning("Skipping callback '%s', unable to load due to: %s" % (callback_name, to_native(e))) + continue self._callbacks_loaded = True diff --git a/test/integration/targets/callback_default/runme.sh b/test/integration/targets/callback_default/runme.sh index d394c7c561..60c0f4231d 100755 --- a/test/integration/targets/callback_default/runme.sh +++ b/test/integration/targets/callback_default/runme.sh @@ -16,6 +16,9 @@ set -eux run_test() { local testname=$1 + # outout was recorded w/o cowsay, ensure we reproduce the same + export ANSIBLE_NOCOWS=1 + # The shenanigans with redirection and 'tee' are to capture STDOUT and # STDERR separately while still displaying both to the console { ansible-playbook -i inventory test.yml \ @@ -33,6 +36,9 @@ run_test_dryrun() { # optional, pass --check to run a dry run local chk=${2:-} + # outout was recorded w/o cowsay, ensure we reproduce the same + export ANSIBLE_NOCOWS=1 + # This needed to satisfy shellcheck that can not accept unquoted variable cmd="ansible-playbook -i inventory ${chk} test_dryrun.yml" diff --git a/test/integration/targets/collections/runme.sh b/test/integration/targets/collections/runme.sh index b5acfafdfb..9fccc83e55 100755 --- a/test/integration/targets/collections/runme.sh +++ b/test/integration/targets/collections/runme.sh @@ -13,7 +13,7 @@ ipath=../../$(basename "${INVENTORY_PATH}") export INVENTORY_PATH="$ipath" # test callback -ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok" +ANSIBLE_LOAD_CALLBACK_PLUGINS=1 ANSIBLE_CALLBACK_WHITELIST=testns.testcoll.usercallback ansible localhost -m ping | grep "usercallback says ok" # test documentation ansible-doc testns.testcoll.testmodule -vvv | grep -- "- normal_doc_frag" |