Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing ansible.builtin FQCNs in hardcoded action names #71824

Merged
merged 22 commits into from Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/71824-action-fqcns.yml
@@ -0,0 +1,2 @@
bugfixes:
- "Adjust various hard-coded action names to also include their ``ansible.builtin.`` and ``ansible.legacy.`` prefixed version (https://github.com/ansible/ansible/issues/71817, https://github.com/ansible/ansible/issues/71818, https://github.com/ansible/ansible/pull/71824)."
4 changes: 2 additions & 2 deletions lib/ansible/cli/adhoc.py
Expand Up @@ -71,7 +71,7 @@ def _play_ds(self, pattern, async_val, poll):
'timeout': context.CLIARGS['task_timeout']}

# avoid adding to tasks that don't support it, unless set, then give user an error
if context.CLIARGS['module_name'] not in ('include_role', 'include_tasks') and any(frozenset((async_val, poll))):
if context.CLIARGS['module_name'] not in C._ACTION_ALL_INCLUDE_ROLE_TASKS and any(frozenset((async_val, poll))):
mytask['async_val'] = async_val
mytask['poll'] = poll

Expand Down Expand Up @@ -120,7 +120,7 @@ def run(self):
raise AnsibleOptionsError(err)

# Avoid modules that don't work with ad-hoc
if context.CLIARGS['module_name'] in ('import_playbook',):
if context.CLIARGS['module_name'] in C._ACTION_IMPORT_PLAYBOOK:
raise AnsibleOptionsError("'%s' is not a valid action for ad-hoc commands"
% context.CLIARGS['module_name'])

Expand Down
2 changes: 1 addition & 1 deletion lib/ansible/cli/console.py
Expand Up @@ -185,7 +185,7 @@ def default(self, arg, forceshell=False):

result = None
try:
check_raw = module in ('command', 'shell', 'script', 'raw')
check_raw = module in C._ACTION_ALLOWS_RAW_ARGS
task = dict(action=dict(module=module, args=parse_kv(module_args, check_raw=check_raw)), timeout=self.task_timeout)
play_ds = dict(
name="Ansible Shell",
Expand Down
3 changes: 2 additions & 1 deletion lib/ansible/cli/playbook.py
Expand Up @@ -8,6 +8,7 @@
import os
import stat

from ansible import constants as C
from ansible import context
from ansible.cli import CLI
from ansible.cli.arguments import option_helpers as opt_help
Expand Down Expand Up @@ -170,7 +171,7 @@ def _process_block(b):
if isinstance(task, Block):
taskmsg += _process_block(task)
else:
if task.action == 'meta' and task.implicit:
if task.action in C._ACTION_META and task.implicit:
continue

all_tags.update(task.tags)
Expand Down
33 changes: 29 additions & 4 deletions lib/ansible/constants.py
Expand Up @@ -17,6 +17,7 @@
from ansible.module_utils.common.collections import Sequence
from ansible.module_utils.parsing.convert_bool import boolean, BOOLEANS_TRUE
from ansible.module_utils.six import string_types
from ansible.utils.fqcn import add_internal_fqcns


def _warning(msg):
Expand Down Expand Up @@ -74,10 +75,10 @@ def __getitem__(self, y):
IGNORE_FILES = ("COPYING", "CONTRIBUTING", "LICENSE", "README", "VERSION", "GUIDELINES") # ignore during module search
INTERNAL_RESULT_KEYS = ('add_host', 'add_group')
LOCALHOST = ('127.0.0.1', 'localhost', '::1')
MODULE_REQUIRE_ARGS = ('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell',
'ansible.windows.win_shell', 'raw', 'script')
MODULE_NO_JSON = ('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell',
'ansible.windows.win_shell', 'raw')
MODULE_REQUIRE_ARGS = tuple(add_internal_fqcns(('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell',
'ansible.windows.win_shell', 'raw', 'script')))
MODULE_NO_JSON = tuple(add_internal_fqcns(('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell',
'ansible.windows.win_shell', 'raw')))
RESTRICTED_RESULT_KEYS = ('ansible_rsync_path', 'ansible_playbook_python', 'ansible_facts')
TREE_DIR = None
VAULT_VERSION_MIN = 1.0
Expand Down Expand Up @@ -164,3 +165,27 @@ def __getitem__(self, y):

for warn in config.WARNINGS:
_warning(warn)


# The following are hard-coded action names
_ACTION_DEBUG = add_internal_fqcns(('debug', ))
_ACTION_IMPORT_PLAYBOOK = add_internal_fqcns(('import_playbook', ))
_ACTION_IMPORT_ROLE = add_internal_fqcns(('import_role', ))
_ACTION_IMPORT_TASKS = add_internal_fqcns(('import_tasks', ))
_ACTION_INCLUDE = add_internal_fqcns(('include', ))
_ACTION_INCLUDE_ROLE = add_internal_fqcns(('include_role', ))
_ACTION_INCLUDE_TASKS = add_internal_fqcns(('include_tasks', ))
_ACTION_INCLUDE_VARS = add_internal_fqcns(('include_vars', ))
_ACTION_META = add_internal_fqcns(('meta', ))
_ACTION_SET_FACT = add_internal_fqcns(('set_fact', ))
_ACTION_HAS_CMD = add_internal_fqcns(('command', 'shell', 'script'))
_ACTION_ALLOWS_RAW_ARGS = _ACTION_HAS_CMD + add_internal_fqcns(('raw', ))
_ACTION_ALL_INCLUDES = _ACTION_INCLUDE + _ACTION_INCLUDE_TASKS + _ACTION_INCLUDE_ROLE
_ACTION_ALL_IMPORT_PLAYBOOKS = _ACTION_INCLUDE + _ACTION_IMPORT_PLAYBOOK
_ACTION_ALL_INCLUDE_IMPORT_TASKS = _ACTION_INCLUDE + _ACTION_INCLUDE_TASKS + _ACTION_IMPORT_TASKS
_ACTION_ALL_PROPER_INCLUDE_IMPORT_ROLES = _ACTION_INCLUDE_ROLE + _ACTION_IMPORT_ROLE
_ACTION_ALL_PROPER_INCLUDE_IMPORT_TASKS = _ACTION_INCLUDE_TASKS + _ACTION_IMPORT_TASKS
_ACTION_ALL_INCLUDE_ROLE_TASKS = _ACTION_INCLUDE_ROLE + _ACTION_INCLUDE_TASKS
_ACTION_ALL_INCLUDE_TASKS = _ACTION_INCLUDE + _ACTION_INCLUDE_TASKS
_ACTION_FACT_GATHERING = add_internal_fqcns(('setup', 'gather_facts'))
_ACTION_WITH_CLEAN_FACTS = _ACTION_SET_FACT + _ACTION_INCLUDE_VARS
8 changes: 4 additions & 4 deletions lib/ansible/executor/task_executor.py
Expand Up @@ -477,7 +477,7 @@ def _execute(self, variables=None):

# if this task is a TaskInclude, we just return now with a success code so the
# main thread can expand the task list for the given host
if self._task.action in ('include', 'include_tasks'):
if self._task.action in C._ACTION_ALL_INCLUDE_TASKS:
include_args = self._task.args.copy()
include_file = include_args.pop('_raw_params', None)
if not include_file:
Expand All @@ -487,7 +487,7 @@ def _execute(self, variables=None):
return dict(include=include_file, include_args=include_args)

# if this task is a IncludeRole, we just return now with a success code so the main thread can expand the task list for the given host
elif self._task.action == 'include_role':
elif self._task.action in C._ACTION_INCLUDE_ROLE:
include_args = self._task.args.copy()
return dict(include_args=include_args)

Expand Down Expand Up @@ -624,7 +624,7 @@ def _evaluate_failed_when_result(result):
return failed_when_result

if 'ansible_facts' in result:
if self._task.action in ('set_fact', 'include_vars'):
if self._task.action in C._ACTION_WITH_CLEAN_FACTS:
vars_copy.update(result['ansible_facts'])
else:
# TODO: cleaning of facts should eventually become part of taskresults instead of vars
Expand Down Expand Up @@ -688,7 +688,7 @@ def _evaluate_failed_when_result(result):
variables[self._task.register] = result = wrap_var(result)

if 'ansible_facts' in result:
if self._task.action in ('set_fact', 'include_vars'):
if self._task.action in C._ACTION_WITH_CLEAN_FACTS:
variables.update(result['ansible_facts'])
else:
# TODO: cleaning of facts should eventually become part of taskresults instead of vars
Expand Down
2 changes: 1 addition & 1 deletion lib/ansible/executor/task_result.py
Expand Up @@ -113,7 +113,7 @@ def clean_copy(self):
result = TaskResult(self._host, self._task, {}, self._task_fields)

# statuses are already reflected on the event type
if result._task and result._task.action in ['debug']:
if result._task and result._task.action in C._ACTION_DEBUG:
# debug is verbose by default to display vars, no need to add invocation
ignore = _IGNORE + ('invocation',)
else:
Expand Down
9 changes: 5 additions & 4 deletions lib/ansible/parsing/mod_args.py
Expand Up @@ -26,13 +26,14 @@
from ansible.parsing.splitter import parse_kv, split_args
from ansible.plugins.loader import module_loader, action_loader
from ansible.template import Templar
from ansible.utils.fqcn import add_internal_fqcns
from ansible.utils.sentinel import Sentinel


# For filtering out modules correctly below
FREEFORM_ACTIONS = frozenset(C.MODULE_REQUIRE_ARGS)

RAW_PARAM_MODULES = FREEFORM_ACTIONS.union((
RAW_PARAM_MODULES = FREEFORM_ACTIONS.union(add_internal_fqcns((
'include',
'include_vars',
'include_tasks',
Expand All @@ -43,16 +44,16 @@
'group_by',
'set_fact',
'meta',
))
)))

BUILTIN_TASKS = frozenset((
BUILTIN_TASKS = frozenset(add_internal_fqcns((
'meta',
'include',
'include_tasks',
'include_role',
'import_tasks',
'import_role'
))
)))


class ModuleArgsParser:
Expand Down
10 changes: 7 additions & 3 deletions lib/ansible/playbook/__init__.py
Expand Up @@ -91,15 +91,19 @@ def _load_playbook_data(self, file_name, variable_manager, vars=None):
self._loader.set_basedir(cur_basedir)
raise AnsibleParserError("playbook entries must be either a valid play or an include statement", obj=entry)

if any(action in entry for action in ('import_playbook', 'include')):
if 'include' in entry:
if any(action in entry for action in C._ACTION_ALL_IMPORT_PLAYBOOKS):
if any(action in entry for action in C._ACTION_INCLUDE):
display.deprecated("'include' for playbook includes. You should use 'import_playbook' instead",
version="2.12", collection_name='ansible.builtin')
pb = PlaybookInclude.load(entry, basedir=self._basedir, variable_manager=variable_manager, loader=self._loader)
if pb is not None:
self._entries.extend(pb._entries)
else:
which = entry.get('import_playbook', entry.get('include', entry))
which = entry
for k in C._ACTION_IMPORT_PLAYBOOK + C._ACTION_INCLUDE:
if k in entry:
which = entry[k]
break
display.display("skipping playbook '%s' due to conditional test failure" % which, color=C.COLOR_SKIP)
else:
entry_obj = Play.load(entry, variable_manager=variable_manager, loader=self._loader, vars=vars)
Expand Down
5 changes: 3 additions & 2 deletions lib/ansible/playbook/block.py
Expand Up @@ -19,6 +19,7 @@
from __future__ import (absolute_import, division, print_function)
__metaclass__ = type

import ansible.constants as C
from ansible.errors import AnsibleParserError
from ansible.playbook.attribute import FieldAttribute
from ansible.playbook.base import Base
Expand Down Expand Up @@ -374,8 +375,8 @@ def evaluate_and_append_task(target):
filtered_block = evaluate_block(task)
if filtered_block.has_tasks():
tmp_list.append(filtered_block)
elif ((task.action == 'meta' and task.implicit) or
(task.action == 'include' and task.evaluate_tags([], self._play.skip_tags, all_vars=all_vars)) or
elif ((task.action in C._ACTION_META and task.implicit) or
(task.action in C._ACTION_INCLUDE and task.evaluate_tags([], self._play.skip_tags, all_vars=all_vars)) or
task.evaluate_tags(self._play.only_tags, self._play.skip_tags, all_vars=all_vars)):
tmp_list.append(task)
return tmp_list
Expand Down
18 changes: 9 additions & 9 deletions lib/ansible/playbook/helpers.py
Expand Up @@ -128,7 +128,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
# But if it wasn't, we can add the yaml object now to get more detail
raise AnsibleParserError(to_native(e), obj=task_ds, orig_exc=e)

if action in ('include', 'import_tasks', 'include_tasks'):
if action in C._ACTION_ALL_INCLUDE_IMPORT_TASKS:

if use_handlers:
include_class = HandlerTaskInclude
Expand All @@ -150,9 +150,9 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
# check to see if this include is dynamic or static:
# 1. the user has set the 'static' option to false or true
# 2. one of the appropriate config options was set
if action == 'include_tasks':
if action in C._ACTION_INCLUDE_TASKS:
is_static = False
elif action == 'import_tasks':
elif action in C._ACTION_IMPORT_TASKS:
is_static = True
elif t.static is not None:
display.deprecated("The use of 'static' has been deprecated. "
Expand All @@ -166,7 +166,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h

if is_static:
if t.loop is not None:
if action == 'import_tasks':
if action in C._ACTION_IMPORT_TASKS:
raise AnsibleParserError("You cannot use loops on 'import_tasks' statements. You should use 'include_tasks' instead.", obj=task_ds)
else:
raise AnsibleParserError("You cannot use 'static' on an include with a loop", obj=task_ds)
Expand Down Expand Up @@ -248,7 +248,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
# nested includes, and we want the include order printed correctly
display.vv("statically imported: %s" % include_file)
except AnsibleFileNotFound:
if action != 'include' or t.static or \
if action not in C._ACTION_INCLUDE or t.static or \
C.DEFAULT_TASK_INCLUDES_STATIC or \
C.DEFAULT_HANDLER_INCLUDES_STATIC and use_handlers:
raise
Expand Down Expand Up @@ -286,7 +286,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
tags = tags.split(',')

if len(tags) > 0:
if action in ('include_tasks', 'import_tasks'):
if action in C._ACTION_ALL_PROPER_INCLUDE_IMPORT_TASKS:
raise AnsibleParserError('You cannot specify "tags" inline to the task, it is a task keyword')
if len(ti_copy.tags) > 0:
raise AnsibleParserError(
Expand Down Expand Up @@ -316,7 +316,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
t.is_static = False
task_list.append(t)

elif action in ('include_role', 'import_role'):
elif action in C._ACTION_ALL_PROPER_INCLUDE_IMPORT_ROLES:
ir = IncludeRole.load(
task_ds,
block=block,
Expand All @@ -329,7 +329,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h
# 1. the user has set the 'static' option to false or true
# 2. one of the appropriate config options was set
is_static = False
if action == 'import_role':
if action in C._ACTION_IMPORT_ROLE:
is_static = True

elif ir.static is not None:
Expand All @@ -340,7 +340,7 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h

if is_static:
if ir.loop is not None:
if action == 'import_role':
if action in C._ACTION_IMPORT_ROLE:
raise AnsibleParserError("You cannot use loops on 'import_role' statements. You should use 'include_role' instead.", obj=task_ds)
else:
raise AnsibleParserError("You cannot use 'static' on an include_role with a loop", obj=task_ds)
Expand Down
5 changes: 3 additions & 2 deletions lib/ansible/playbook/included_file.py
Expand Up @@ -21,6 +21,7 @@

import os

from ansible import constants as C
from ansible.errors import AnsibleError
from ansible.module_utils._text import to_text
from ansible.playbook.task_include import TaskInclude
Expand Down Expand Up @@ -67,7 +68,7 @@ def process_include_results(results, iterator, loader, variable_manager):
original_host = res._host
original_task = res._task

if original_task.action in ('include', 'include_tasks', 'include_role'):
if original_task.action in C._ACTION_ALL_INCLUDES:
if original_task.loop:
if 'results' not in res._result:
continue
Expand Down Expand Up @@ -111,7 +112,7 @@ def process_include_results(results, iterator, loader, variable_manager):

templar = Templar(loader=loader, variables=task_vars)

if original_task.action in ('include', 'include_tasks'):
if original_task.action in C._ACTION_ALL_INCLUDE_TASKS:
include_file = None
if original_task:
if original_task.static:
Expand Down
3 changes: 2 additions & 1 deletion lib/ansible/playbook/playbook_include.py
Expand Up @@ -21,6 +21,7 @@

import os

import ansible.constants as C
from ansible.errors import AnsibleParserError, AnsibleAssertionError
from ansible.module_utils._text import to_bytes
from ansible.module_utils.six import iteritems, string_types
Expand Down Expand Up @@ -139,7 +140,7 @@ def preprocess_data(self, ds):
new_ds.ansible_pos = ds.ansible_pos

for (k, v) in iteritems(ds):
if k in ('include', 'import_playbook'):
if k in C._ACTION_ALL_IMPORT_PLAYBOOKS:
self._preprocess_import(ds, new_ds, k, v)
else:
# some basic error checking, to make sure vars are properly
Expand Down
5 changes: 3 additions & 2 deletions lib/ansible/playbook/role_include.py
Expand Up @@ -20,6 +20,7 @@

from os.path import basename

import ansible.constants as C
from ansible.errors import AnsibleParserError
from ansible.playbook.attribute import FieldAttribute
from ansible.playbook.block import Block
Expand Down Expand Up @@ -127,7 +128,7 @@ def load(data, block=None, role=None, task_include=None, variable_manager=None,
if ir._role_name is None:
raise AnsibleParserError("'name' is a required field for %s." % ir.action, obj=data)

if 'public' in ir.args and ir.action != 'include_role':
if 'public' in ir.args and ir.action not in C._ACTION_INCLUDE_ROLE:
raise AnsibleParserError('Invalid options for %s: public' % ir.action, obj=data)

# validate bad args, otherwise we silently ignore
Expand All @@ -144,7 +145,7 @@ def load(data, block=None, role=None, task_include=None, variable_manager=None,
ir._from_files[from_key] = basename(args_value)

apply_attrs = ir.args.get('apply', {})
if apply_attrs and ir.action != 'include_role':
if apply_attrs and ir.action not in C._ACTION_INCLUDE_ROLE:
raise AnsibleParserError('Invalid options for %s: apply' % ir.action, obj=data)
elif not isinstance(apply_attrs, dict):
raise AnsibleParserError('Expected a dict for apply but got %s instead' % type(apply_attrs), obj=data)
Expand Down