Various cleanup to run_command

* Rename fail_on_rc_non_zero to check_rc, much more succinct.
* Simplify method defintion
* Fix command module and drop shell=shell option; whether to use
  shell is determined by if args is a list.

lib/ansible/module_common.py

@@ -677,40 +677,34 @@ class AnsibleModule(object):
                 self.set_context_if_different(src, context, False)
         os.rename(src, dest)
 
-    def run_command(self, args, **kwargs):
+    def run_command(self, args, check_rc=False, close_fds=False, executable=None):
         '''
         Execute a command, returns rc, stdout, and stderr.
         args is the command to run
         If args is a list, the command will be run with shell=False.
         Otherwise, the command will be run with shell=True when args is a string.
-        kwargs is a dict of keyword arguments:
+        Other arguments:
-        - fail_on_rc_non_zero (boolean) Whether to call fail_json in case of
+        - check_rc (boolean)  Whether to call fail_json in case of
-                                        non zero RC.  Default is False.
+                              non zero RC.  Default is False.
         - close_fds (boolean) See documentation for subprocess.Popen().
                               Default is False.
         - executable (string) See documentation for subprocess.Popen().
                               Default is None.
         '''
         if isinstance(args, list):
-            kwargs['shell'] = False
+            shell = False
         elif isinstance(args, basestring):
-            kwargs['shell'] = True
+            shell = True
         else:
             msg = "Argument 'args' to run_command must be list or string"
             self.fail_json(rc=257, cmd=args, msg=msg)
-        if 'fail_on_rc_non_zero' not in kwargs:
-            kwargs['fail_on_rc_non_zero'] = False
-        if 'close_fds' not in kwargs:
-            kwargs['close_fds'] = False
-        if 'executable' not in kwargs:
-            kwargs['executable'] = None
         rc = 0
         msg = None
         try:
             cmd = subprocess.Popen(args,
-                                   executable=kwargs['executable'],
+                                   executable=executable,
-                                   shell=kwargs['shell'],
+                                   shell=shell,
-                                   close_fds=kwargs['close_fds'],
+                                   close_fds=close_fds,
                                    stdout=subprocess.PIPE, stderr=subprocess.PIPE)
             out, err = cmd.communicate()
             rc = cmd.returncode
@@ -718,7 +712,7 @@ class AnsibleModule(object):
             self.fail_json(rc=e.errno, msg=str(e), cmd=args)
         except:
             self.fail_json(rc=257, msg=traceback.format_exc(), cmd=args)
-        if rc != 0 and kwargs['fail_on_rc_non_zero']:
+        if rc != 0 and check_rc:
             msg = err.rstrip()
             self.fail_json(cmd=args, rc=rc, stdout=out, stderr=err, msg=msg)
         return (rc, out, err)

library/command

@@ -99,7 +99,7 @@ def main():
         args = shlex.split(args)
     startd = datetime.datetime.now()
 
-    rc, out, err = module.run_command(args, shell=shell, executable=executable)
+    rc, out, err = module.run_command(args, executable=executable)
 
     endd = datetime.datetime.now()
     delta = endd - startd

library/facter

@@ -44,7 +44,7 @@ def main():
     )
 
     cmd = ["/usr/bin/env", "facter", "--json"]
-    rc, out, err = module.run_command(cmd, fail_on_rc_non_zero=True)
+    rc, out, err = module.run_command(cmd, check_rc=True)
     module.exit_json(**json.loads(out))
 
 # this is magic, see lib/ansible/module_common.py

library/git

@@ -81,7 +81,7 @@ def clone(module, repo, dest, remote):
         pass
     os.chdir(dest_dirname)
     return module.run_command("git clone -o %s %s %s" % (remote, repo, dest),
-                              fail_on_rc_non_zero=True)
+                              check_rc=True)
 
 def has_local_mods(dest):
     os.chdir(dest)
@@ -99,7 +99,7 @@ def reset(module,dest,force):
     os.chdir(dest)
     if not force and has_local_mods(dest):
         module.fail_json(msg="Local modifications exist in repository (force=no).")
-    return module.run_command("git reset --hard HEAD", fail_on_rc_non_zero=True)
+    return module.run_command("git reset --hard HEAD", check_rc=True)
 
 def get_branches(module, dest):
     os.chdir(dest)
@@ -210,7 +210,7 @@ def switch_version(module, dest, remote, version):
         if rc != 0:
             module.fail_json(msg="Failed to checkout branch %s" % branch)
         cmd = "git reset --hard %s" % remote
-    return module.run_command(cmd, fail_on_rc_non_zero=True)
+    return module.run_command(cmd, check_rc=True)
 
 # ===========================================
 

library/ohai

@@ -43,7 +43,7 @@ def main():
         argument_spec = dict()
     )
     cmd = ["/usr/bin/env", "ohai"]
-    rc, out, err = module.run_command(cmd, fail_on_rc_non_zero=True)
+    rc, out, err = module.run_command(cmd, check_rc=True)
     module.exit_json(**json.loads(out))
 
 # this is magic, see lib/ansible/module_common.py

library/subversion

@@ -85,7 +85,7 @@ class Subversion(object):
         if self.password:
             bits.append("--password '%s'" % self.password)
         bits.append(args)
-        rc, out, err = self.module.run_command(' '.join(bits), fail_on_rc_non_zero=True)
+        rc, out, err = self.module.run_command(' '.join(bits), check_rc=True)
         return out.splitlines()
 
     def checkout(self):

library/supervisorctl

@@ -63,7 +63,7 @@ def main():
 
     if state == 'present':
         if not present:
-            module.run_command('%s reread' % SUPERVISORCTL, fail_on_rc_non_zero=True)
+            module.run_command('%s reread' % SUPERVISORCTL, check_rc=True)
             rc, out, err = module.run_command('%s add %s' % (SUPERVISORCTL, name))
 
             if '%s: added process group' % name in out: