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.

Sign up for GitHub

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

Jump to bottom

WIP DNM Handle network platform action plugin collection searching #61489

Closed
wants to merge 5 commits into from
Closed

WIP DNM Handle network platform action plugin collection searching #61489

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f7f408
Handle network platform action plugin collection searching
Aug 28, 2019
3b8aebf
Use constant
Aug 28, 2019
8d4e0d5
rpartition
Aug 28, 2019
768ff9d
emptyline
Aug 28, 2019
febfd00
In case handler was not found in collection list
Aug 28, 2019
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
81 changes: 69 additions & 12 deletions lib/ansible/executor/task_executor.py
View file
Open in desktop
Expand Up @@ -1015,24 +1015,81 @@ def _set_connection_options(self, variables, templar):
self._play_context.prompt = self._connection.become.prompt

def _get_action_handler(self, connection, templar):
'''
"""
Returns the correct action plugin to handle the requestion task action
'''

module_prefix = self._task.action.split('_')[0]

"""
action_loader = self._shared_loader_obj.action_loader
collections = self._task.collections

# let action plugin override module, fallback to 'normal' action plugin otherwise
if self._shared_loader_obj.action_loader.has_plugin(self._task.action, collection_list=collections):
collection_from_task, _, module_name = self._task.action.rpartition(".")
module_prefix = module_name.split("_")[0]
if collection_from_task:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to merge the two cases with something like this?

if collections is None and collection_from_task:
    collections = [collection_from_task]

Now collections is either the non-None contents of collections, or the task's collection, or None if neither exist, which means we can drop the collections check from the all()s and just use collections regardless

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be worried about changing the collection list in this case for a non-network module....
(because we would have changed the collection when we didn't have a network module match)

Copy link
Author

@cidrblock cidrblock Aug 28, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that might be ok though, I don't think the first case below (non network related) would pick up a handler in the same collection as the module unless the user explicitly listed that collection...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and IMHO, we should be using the handler in the collection local to the module by default if one exists...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I hadn't noticed collections isn't added here.

And yeah, we should probably be using something like collection_list=[collection_from_task] + collections to look up the action plugin (resolving appropriately when one or both don't exist). The two paths are just so similar to each other I'd really rather avoid keeping them separate unless there's a good reason to

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. That raises an interesting question. If a user provides a collection list, should we search that before or after the collection local to the module. I would think we search the collection local to the module after explicitly listed collections but before defaulting to a core found plugin. I can't see how to do that without 2 steps. Thoughts?

derived_collections = [collection_from_task]
else:
derived_collections = None

# let action plugin override module, fallback to 'normal'
# action plugin otherwise
if action_loader.has_plugin(
self._task.action, collection_list=collections
):
handler_name = self._task.action
# FIXME: is this code path even live anymore? check w/ networking folks; it trips sometimes when it shouldn't
elif all((module_prefix in C.NETWORK_GROUP_MODULES, module_prefix in self._shared_loader_obj.action_loader)):

# module prefix is network and a list of collections provided
# this would return the core plugin if checked independant of
# collections
elif all(
(
collections,
module_prefix in C.NETWORK_GROUP_MODULES,
action_loader.has_plugin(
module_prefix, collection_list=collections
),
)
):

handler_name = module_prefix
display.vvv(
"Found network platform handler using module prefix with"
" collection list: %s"
% action_loader.find_plugin(
module_prefix, collection_list=collections
),
host=self._play_context.remote_addr,
)

# module prefix is network and a handler wasn't found previously
# default to using the module's local collection
# set the tasks collection to the module's collection name
elif all(
(
module_prefix in C.NETWORK_GROUP_MODULES,
action_loader.has_plugin(
module_prefix,
collection_list=derived_collections,
),
)
):
handler_name = module_prefix
collections = derived_collections
display.vvv(
"Found network platform handler using module prefix, "
" handler was not found in provided collection list"
" or provided collection list was empty."
" Collection derived from task: %s"
% action_loader.find_plugin(
module_prefix, collection_list=collections
),
host=self._play_context.remote_addr,
)

else:
# FUTURE: once we're comfortable with collections impl, preface this action with ansible.builtin so it can't be hijacked
handler_name = 'normal'
collections = None # until then, we don't want the task's collection list to be consulted; use the builtin
# FUTURE: once we're comfortable with collections impl, preface
# this action with ansible.builtin so it can't be hijacked
handler_name = "normal"
# # until then, we don't want the task's collection list
# to be consulted; use the builtin
collections = None

handler = self._shared_loader_obj.action_loader.get(
handler_name,
Expand Down