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

@@ -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)
 

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

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)

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 = {}

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

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)

test/units/parsing/test_mod_args.py

@@ -1,129 +1,128 @@
 # (c) 2012-2014, Michael DeHaan <michael.dehaan@gmail.com>
-#
-# 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 <http://www.gnu.org/licenses/>.
+# 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)

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):