From 235bdfb9963e7f33ce72d52d1396830ea37e9c53 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Thu, 21 Dec 2017 10:45:33 -0800 Subject: [PATCH] Remove shell module specialcases Shell is implemented via the command module. There was a special case in mod_args to do that. Make shell into an action plugin to handle that instead. Also move the special case for the command nanny into a command module action plugin. This is more appropriate as we then do not have to send a parameter that is only for the command module to every single module. --- lib/ansible/module_utils/basic.py | 6 +- lib/ansible/modules/commands/command.py | 6 +- lib/ansible/parsing/mod_args.py | 18 ---- lib/ansible/plugins/action/__init__.py | 3 - lib/ansible/plugins/action/command.py | 25 +++++ lib/ansible/plugins/action/shell.py | 25 +++++ test/units/parsing/test_mod_args.py | 123 ++++++++++++------------ test/units/playbook/test_task.py | 22 ++--- 8 files changed, 125 insertions(+), 103 deletions(-) create mode 100644 lib/ansible/plugins/action/command.py create mode 100644 lib/ansible/plugins/action/shell.py diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 82d1217b80..a7eef260aa 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -812,12 +812,11 @@ class AnsibleModule(object): self._warnings = [] self._deprecations = [] self._clean = {} - self._command_warn = True self.aliases = {} self._legal_inputs = ['_ansible_check_mode', '_ansible_no_log', '_ansible_debug', '_ansible_diff', '_ansible_verbosity', '_ansible_selinux_special_fs', '_ansible_module_name', '_ansible_version', '_ansible_syslog_facility', - '_ansible_socket', '_ansible_shell_executable', '_ansible_command_warnings'] + '_ansible_socket', '_ansible_shell_executable'] self._options_context = list() if add_file_common_args: @@ -1630,9 +1629,6 @@ class AnsibleModule(object): elif k == '_ansible_shell_executable' and v: self._shell = v - elif k == '_ansible_command_warnings' and v: - self._command_warn = v - elif check_invalid_arguments and k not in legal_inputs: unsupported_parameters.add(k) diff --git a/lib/ansible/modules/commands/command.py b/lib/ansible/modules/commands/command.py index 73153dd2dd..f92a4689ac 100644 --- a/lib/ansible/modules/commands/command.py +++ b/lib/ansible/modules/commands/command.py @@ -165,7 +165,8 @@ def main(): executable=dict(), creates=dict(type='path'), removes=dict(type='path'), - warn=dict(type='bool'), + # The default for this really comes from the action plugin + warn=dict(type='bool', default=True), stdin=dict(required=False), ) ) @@ -179,9 +180,6 @@ def main(): warn = module.params['warn'] stdin = module.params['stdin'] - if warn is None: - warn = module._command_warn - if not shell and executable: module.warn("As of Ansible 2.4, the parameter 'executable' is no longer supported with the 'command' module. Not using '%s'." % executable) executable = None diff --git a/lib/ansible/parsing/mod_args.py b/lib/ansible/parsing/mod_args.py index baf9e6b457..97d0f9c98f 100644 --- a/lib/ansible/parsing/mod_args.py +++ b/lib/ansible/parsing/mod_args.py @@ -116,21 +116,6 @@ class ModuleArgsParser: else: return (tokens[0], "") - def _handle_shell_weirdness(self, action, args): - ''' - given an action name and an args dictionary, return the - proper action name and args dictionary. This mostly is due - to shell/command being treated special and nothing else - ''' - - # the shell module really is the command module with an additional - # parameter - if action == 'shell': - action = 'command' - args['_uses_shell'] = True - - return (action, args) - def _normalize_parameters(self, thing, action=None, additional_args=None): ''' arguments can be fuzzy. Deal with all the forms. @@ -319,7 +304,4 @@ class ModuleArgsParser: ", ".join(RAW_PARAM_MODULES)), obj=self._task_ds) - # shell modules require special handling - (action, args) = self._handle_shell_weirdness(action, args) - return (action, args, delegate_to) diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 531b12e6b9..987f62f388 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -612,9 +612,6 @@ class ActionBase(with_metaclass(ABCMeta, object)): # make sure all commands use the designated shell executable module_args['_ansible_shell_executable'] = self._play_context.executable - # let command know it should avoid specific warnings - module_args['_ansible_command_warnings'] = C.COMMAND_WARNINGS - def _update_connection_options(self, options, variables=None): ''' ensures connections have the appropriate information ''' update = {} diff --git a/lib/ansible/plugins/action/command.py b/lib/ansible/plugins/action/command.py new file mode 100644 index 0000000000..87775f3ca9 --- /dev/null +++ b/lib/ansible/plugins/action/command.py @@ -0,0 +1,25 @@ +# Copyright: (c) 2017, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +from ansible import constants as C +from ansible.plugins.action import ActionBase +from ansible.utils.vars import merge_hash + + +class ActionModule(ActionBase): + + def run(self, tmp=None, task_vars=None): + self._supports_async = True + results = super(ActionModule, self).run(tmp, task_vars) + + # Command module has a special config option to turn off the command nanny warnings + if 'warn' not in self._task.args: + self._task.args['warn'] = C.COMMAND_WARNINGS + + wrap_async = self._task.async_val and not self._connection.has_native_async + results = merge_hash(results, self._execute_module(tmp=tmp, task_vars=task_vars, wrap_async=wrap_async)) + + return results diff --git a/lib/ansible/plugins/action/shell.py b/lib/ansible/plugins/action/shell.py new file mode 100644 index 0000000000..3e464721bf --- /dev/null +++ b/lib/ansible/plugins/action/shell.py @@ -0,0 +1,25 @@ +# Copyright: (c) 2017, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + +from ansible.plugins.action import ActionBase +from ansible.utils.vars import merge_hash + + +class ActionModule(ActionBase): + + def run(self, tmp=None, task_vars=None): + # Shell module is implemented via command + self._task.action = 'command' + self._task.args['_uses_shell'] = True + + command_action = self._shared_loader_obj.action_loader.get('command', + task=self._task, + connection=self._connection, + play_context=self._play_context, + loader=self._loader, + templar=self._templar, + shared_loader_obj=self._shared_loader_obj) + return command_action.run(task_vars=task_vars) diff --git a/test/units/parsing/test_mod_args.py b/test/units/parsing/test_mod_args.py index d644813feb..a4cd4e77b6 100644 --- a/test/units/parsing/test_mod_args.py +++ b/test/units/parsing/test_mod_args.py @@ -1,129 +1,128 @@ # (c) 2012-2014, Michael DeHaan -# -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . +# Copyright 2017, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -# Make coding more python3-ish -from __future__ import (absolute_import, division, print_function) +from __future__ import absolute_import, division, print_function __metaclass__ = type -from ansible.compat.tests import unittest +import pytest + from ansible.errors import AnsibleParserError from ansible.parsing.mod_args import ModuleArgsParser -class TestModArgsDwim(unittest.TestCase): +class TestModArgsDwim: # TODO: add tests that construct ModuleArgsParser with a task reference # TODO: verify the AnsibleError raised on failure knows the task # and the task knows the line numbers - def setUp(self): - pass + INVALID_MULTIPLE_ACTIONS = ( + ({'action': 'shell echo hi', 'local_action': 'shell echo hi'}, "action and local_action are mutually exclusive"), + ({'action': 'shell echo hi', 'shell': 'echo hi'}, "conflicting action statements: shell, shell"), + ({'local_action': 'shell echo hi', 'shell': 'echo hi'}, "conflicting action statements: shell, shell"), + ) def _debug(self, mod, args, to): print("RETURNED module = {0}".format(mod)) print(" args = {0}".format(args)) print(" to = {0}".format(to)) - def tearDown(self): - pass - def test_basic_shell(self): m = ModuleArgsParser(dict(shell='echo hi')) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'command') - self.assertEqual(args, dict( + + assert mod == 'shell' + assert args == dict( _raw_params='echo hi', - _uses_shell=True, - )) - self.assertIsNone(to) + ) + assert to is None def test_basic_command(self): m = ModuleArgsParser(dict(command='echo hi')) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'command') - self.assertEqual(args, dict( + + assert mod == 'command' + assert args == dict( _raw_params='echo hi', - )) - self.assertIsNone(to) + ) + assert to is None def test_shell_with_modifiers(self): m = ModuleArgsParser(dict(shell='/bin/foo creates=/tmp/baz removes=/tmp/bleep')) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'command') - self.assertEqual(args, dict( + + assert mod == 'shell' + assert args == dict( creates='/tmp/baz', removes='/tmp/bleep', _raw_params='/bin/foo', - _uses_shell=True, - )) - self.assertIsNone(to) + ) + assert to is None def test_normal_usage(self): m = ModuleArgsParser(dict(copy='src=a dest=b')) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'copy') - self.assertEqual(args, dict(src='a', dest='b')) - self.assertIsNone(to) + + assert mod, 'copy' + assert args, dict(src='a', dest='b') + assert to is None def test_complex_args(self): m = ModuleArgsParser(dict(copy=dict(src='a', dest='b'))) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'copy') - self.assertEqual(args, dict(src='a', dest='b')) - self.assertIsNone(to) + + assert mod, 'copy' + assert args, dict(src='a', dest='b') + assert to is None def test_action_with_complex(self): m = ModuleArgsParser(dict(action=dict(module='copy', src='a', dest='b'))) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'copy') - self.assertEqual(args, dict(src='a', dest='b')) - self.assertIsNone(to) + + assert mod == 'copy' + assert args == dict(src='a', dest='b') + assert to is None def test_action_with_complex_and_complex_args(self): m = ModuleArgsParser(dict(action=dict(module='copy', args=dict(src='a', dest='b')))) mod, args, to = m.parse() self._debug(mod, args, to) - self.assertEqual(mod, 'copy') - self.assertEqual(args, dict(src='a', dest='b')) - self.assertIsNone(to) + + assert mod == 'copy' + assert args == dict(src='a', dest='b') + assert to is None def test_local_action_string(self): m = ModuleArgsParser(dict(local_action='copy src=a dest=b')) mod, args, delegate_to = m.parse() self._debug(mod, args, delegate_to) - self.assertEqual(mod, 'copy') - self.assertEqual(args, dict(src='a', dest='b')) - self.assertIs(delegate_to, 'localhost') + + assert mod == 'copy' + assert args == dict(src='a', dest='b') + assert delegate_to == 'localhost' + + @pytest.mark.parametrize("args_dict, msg", INVALID_MULTIPLE_ACTIONS) + def test_multiple_actions(self, args_dict, msg): + m = ModuleArgsParser(args_dict) + with pytest.raises(AnsibleParserError) as err: + m.parse() + + assert err.value.args[0] == msg def test_multiple_actions(self): - m = ModuleArgsParser(dict(action='shell echo hi', local_action='shell echo hi')) - self.assertRaises(AnsibleParserError, m.parse) + args_dict = {'ping': 'data=hi', 'shell': 'echo hi'} + m = ModuleArgsParser(args_dict) + with pytest.raises(AnsibleParserError) as err: + m.parse() - m = ModuleArgsParser(dict(action='shell echo hi', shell='echo hi')) - self.assertRaises(AnsibleParserError, 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')) - m = ModuleArgsParser(dict(local_action='shell echo hi', shell='echo hi')) - self.assertRaises(AnsibleParserError, m.parse) - - m = ModuleArgsParser(dict(ping='data=hi', shell='echo hi')) - self.assertRaises(AnsibleParserError, m.parse) diff --git a/test/units/playbook/test_task.py b/test/units/playbook/test_task.py index 662c6b7931..3f1f17fd48 100644 --- a/test/units/playbook/test_task.py +++ b/test/units/playbook/test_task.py @@ -23,13 +23,13 @@ from ansible.compat.tests import unittest from ansible.playbook.task import Task -basic_shell_task = dict( +basic_command_task = dict( name='Test Task', - shell='echo hi' + command='echo hi' ) -kv_shell_task = dict( - action='shell echo hi' +kv_command_task = dict( + action='command echo hi' ) @@ -54,20 +54,20 @@ class TestTask(unittest.TestCase): pass def test_load_task_simple(self): - t = Task.load(basic_shell_task) + t = Task.load(basic_command_task) assert t is not None - self.assertEqual(t.name, basic_shell_task['name']) + self.assertEqual(t.name, basic_command_task['name']) self.assertEqual(t.action, 'command') - self.assertEqual(t.args, dict(_raw_params='echo hi', _uses_shell=True)) + self.assertEqual(t.args, dict(_raw_params='echo hi')) def test_load_task_kv_form(self): - t = Task.load(kv_shell_task) + t = Task.load(kv_command_task) self.assertEqual(t.action, 'command') - self.assertEqual(t.args, dict(_raw_params='echo hi', _uses_shell=True)) + self.assertEqual(t.args, dict(_raw_params='echo hi')) def test_task_auto_name(self): - assert 'name' not in kv_shell_task - t = Task.load(kv_shell_task) + assert 'name' not in kv_command_task + t = Task.load(kv_command_task) # self.assertEqual(t.name, 'shell echo hi') def test_task_auto_name_with_role(self):