diff --git a/changelogs/fragments/mod_args_tweaks.yml b/changelogs/fragments/mod_args_tweaks.yml new file mode 100644 index 00000000000..27c3e803513 --- /dev/null +++ b/changelogs/fragments/mod_args_tweaks.yml @@ -0,0 +1,2 @@ +minor_changes: +- changed task module/action parsing to report more helpful errors diff --git a/lib/ansible/parsing/mod_args.py b/lib/ansible/parsing/mod_args.py index 9cffb53a42a..da1ab26db0c 100644 --- a/lib/ansible/parsing/mod_args.py +++ b/lib/ansible/parsing/mod_args.py @@ -115,6 +115,15 @@ class ModuleArgsParser: raise AnsibleAssertionError("the type of 'task_ds' should be a dict, but is a %s" % type(task_ds)) self._task_ds = task_ds self._collection_list = collection_list + # delayed local imports to prevent circular import + from ansible.playbook.task import Task + from ansible.playbook.handler import Handler + # store the valid Task/Handler attrs for quick access + self._task_attrs = set(Task._valid_attrs.keys()) + self._task_attrs.update(set(Handler._valid_attrs.keys())) + # HACK: why is static not a FieldAttribute on task with a post-validate to bomb if not include/import? + self._task_attrs.add('static') + self._task_attrs = frozenset(self._task_attrs) def _split_module_string(self, module_string): ''' @@ -286,8 +295,11 @@ class ModuleArgsParser: # module: is the more new-style invocation - # walk the input dictionary to see we recognize a module name - for (item, value) in iteritems(self._task_ds): + # filter out task attributes so we're only querying unrecognized keys as actions/modules + non_task_ds = dict((k, v) for k, v in iteritems(self._task_ds) if (k not in self._task_attrs) and (not k.startswith('with_'))) + + # walk the filtered input dictionary to see if we recognize a module name + for item, value in iteritems(non_task_ds): if item in BUILTIN_TASKS or action_loader.has_plugin(item, collection_list=self._collection_list) or \ module_loader.has_plugin(item, collection_list=self._collection_list): # finding more than one module name is a problem @@ -299,14 +311,13 @@ class ModuleArgsParser: # if we didn't see any module in the task at all, it's not a task really if action is None: - if 'ping' not in module_loader: - raise AnsibleParserError("The requested action was not found in configured module paths. " - "Additionally, core modules are missing. If this is a checkout, " - "run 'git pull --rebase' to correct this problem.", + if non_task_ds: # there was one non-task action, but we couldn't find it + bad_action = list(non_task_ds.keys())[0] + raise AnsibleParserError("couldn't resolve module/action '{0}'. This often indicates a " + "misspelling, missing collection, or incorrect module path.".format(bad_action), obj=self._task_ds) - else: - raise AnsibleParserError("no action detected in task. This often indicates a misspelled module name, or incorrect module path.", + raise AnsibleParserError("no module/action detected in task.", obj=self._task_ds) elif args.get('_raw_params', '') != '' and action not in RAW_PARAM_MODULES: templar = Templar(loader=None) diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 39c3a2be92e..624eeffb47a 100644 --- a/lib/ansible/plugins/loader.py +++ b/lib/ansible/plugins/loader.py @@ -375,7 +375,7 @@ class PluginLoader: return to_text(found_files[0]) - def _find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None): + def find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None): ''' Find a plugin named name ''' global _PLUGIN_FILTERS @@ -498,20 +498,6 @@ class PluginLoader: return None - def find_plugin(self, name, mod_type='', ignore_deprecated=False, check_aliases=False, collection_list=None): - ''' Find a plugin named name ''' - - # Import here to avoid circular import - from ansible.vars.reserved import is_reserved_name - - plugin = self._find_plugin(name, mod_type=mod_type, ignore_deprecated=ignore_deprecated, check_aliases=check_aliases, collection_list=collection_list) - if plugin and self.package == 'ansible.modules' and name not in ('gather_facts',) and is_reserved_name(name): - raise AnsibleError( - 'Module "%s" shadows the name of a reserved keyword. Please rename or remove this module. Found at %s' % (name, plugin) - ) - - return plugin - def has_plugin(self, name, collection_list=None): ''' Checks if a plugin named name exists ''' diff --git a/test/integration/targets/shadowed_module/aliases b/test/integration/targets/shadowed_module/aliases deleted file mode 100644 index b59832142f2..00000000000 --- a/test/integration/targets/shadowed_module/aliases +++ /dev/null @@ -1 +0,0 @@ -shippable/posix/group3 diff --git a/test/integration/targets/shadowed_module/library/tags b/test/integration/targets/shadowed_module/library/tags deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/test/integration/targets/shadowed_module/playbook.yml b/test/integration/targets/shadowed_module/playbook.yml deleted file mode 100644 index 4823721d11d..00000000000 --- a/test/integration/targets/shadowed_module/playbook.yml +++ /dev/null @@ -1,6 +0,0 @@ ---- -- hosts: localhost - gather_facts: false - tasks: - - command: whoami - tags: foo diff --git a/test/integration/targets/shadowed_module/playbook_lookup.yml b/test/integration/targets/shadowed_module/playbook_lookup.yml deleted file mode 100644 index 6bee31a3da7..00000000000 --- a/test/integration/targets/shadowed_module/playbook_lookup.yml +++ /dev/null @@ -1,6 +0,0 @@ ---- -- hosts: localhost - gather_facts: false - tasks: - - debug: - msg: "{{ lookup('vars', 'inventory_hostname') }}" diff --git a/test/integration/targets/shadowed_module/runme.sh b/test/integration/targets/shadowed_module/runme.sh deleted file mode 100755 index 4703140094a..00000000000 --- a/test/integration/targets/shadowed_module/runme.sh +++ /dev/null @@ -1,14 +0,0 @@ -#!/usr/bin/env bash - -set -ux - -OUT=$(ansible-playbook playbook.yml -i inventory -e @../../integration_config.yml "$@" 2>&1 | grep 'ERROR! Module "tags" shadows the name of a reserved keyword.') - -if [[ -z "$OUT" ]]; then - echo "Fake tags module did not cause error" - exit 1 -fi - -# This playbook calls a lookup which shadows a keyword. -# This is an ok situation, and should not error -ansible-playbook playbook_lookup.yml -i ../../inventory -e @../../integration_config.yml "$@" diff --git a/test/units/parsing/test_mod_args.py b/test/units/parsing/test_mod_args.py index 7aa2161e427..50c3b3315e4 100644 --- a/test/units/parsing/test_mod_args.py +++ b/test/units/parsing/test_mod_args.py @@ -6,6 +6,7 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type import pytest +import re from ansible.errors import AnsibleParserError from ansible.parsing.mod_args import ModuleArgsParser @@ -124,5 +125,13 @@ class TestModArgsDwim: m.parse() assert err.value.args[0].startswith("conflicting action statements: ") - conflicts = set(err.value.args[0][len("conflicting action statements: "):].split(', ')) - assert conflicts == set(('ping', 'shell')) + actions = set(re.search(r'(\w+), (\w+)', err.value.args[0]).groups()) + assert actions == set(['ping', 'shell']) + + def test_bogus_action(self): + args_dict = {'bogusaction': {}} + m = ModuleArgsParser(args_dict) + with pytest.raises(AnsibleParserError) as err: + m.parse() + + assert err.value.args[0].startswith("couldn't resolve module/action 'bogusaction'") diff --git a/test/units/playbook/test_helpers.py b/test/units/playbook/test_helpers.py index 22b2718c39b..4c26ee979ae 100644 --- a/test/units/playbook/test_helpers.py +++ b/test/units/playbook/test_helpers.py @@ -108,7 +108,7 @@ class TestLoadListOfTasks(unittest.TestCase, MixinForMocks): def test_empty_task(self): ds = [{}] self.assertRaisesRegexp(errors.AnsibleParserError, - "no action detected in task. This often indicates a misspelled module name, or incorrect module path", + "no module/action detected in task", helpers.load_list_of_tasks, ds, play=self.mock_play, variable_manager=self.mock_variable_manager, loader=self.fake_loader) @@ -116,7 +116,7 @@ class TestLoadListOfTasks(unittest.TestCase, MixinForMocks): def test_empty_task_use_handlers(self): ds = [{}] self.assertRaisesRegexp(errors.AnsibleParserError, - "no action detected in task. This often indicates a misspelled module name, or incorrect module path", + "no module/action detected in task.", helpers.load_list_of_tasks, ds, use_handlers=True, @@ -384,7 +384,7 @@ class TestLoadListOfBlocks(unittest.TestCase, MixinForMocks): ds = [{}] mock_play = MagicMock(name='MockPlay') self.assertRaisesRegexp(errors.AnsibleParserError, - "no action detected in task. This often indicates a misspelled module name, or incorrect module path", + "no module/action detected in task", helpers.load_list_of_blocks, ds, mock_play, parent_block=None,