Issue 2200 - PART I/II - Migrate to argparse

run.py

Index: run.py
===================================================================
--- a/run.py
+++ b/run.py
@@ -1,6 +1,7 @@
 #!/usr/bin/env python
 # coding: utf-8
 
+import argparse
 import sys
 import os
 import re
@@ -8,39 +9,43 @@
 import getopt
 import yaml
 
-def usage():
-  print >>sys.stderr, '''
-Usage: %s [-u <user>] [-h <host>|<group>] [-i] ... <command>
+def createArgumentParser(**kwargs):
+  parser = argparse.ArgumentParser(**kwargs)
+  parser.add_argument(
+    '-u', metavar='<user>', dest='user', type=str, default='vagrant',
+    help='user name for use with SSH, must exist on all hosts'
+  )
 
-Runs a command on the given hosts or groups of hosts.
+  parser.add_argument(
+    '-?', action='help',
+    help='print this message and exit'
+  )
 

[Wladimir Palant, 2015/04/07 14:58:04]

This is just wrong...

1) This assumes add_help=False parameter, yet you require the caller to pass it
in explicitly.

2) Why even do this? Just let argparse take care of it. This will be more
consistent anyway - -? is usually used on Windows, Unix tools should support
--help.

[mathias, 2015/04/07 15:36:51]

I just tried to stay consistent with the existing arguments for now. But yes
this makes things more complicated. Thus I've now removed these kludges, so the
default handling can apply again. Though that required re-naming '-h' into '-t'
and '--target' (in run.py).

-Options:
-  -u <user>       User name to use with the SSH command
-  -h <host|group> Host or group to run the command on (can be specified multiple times)
-  -i              If specified, command will be executed on all hosts despite errors
-''' % sys.argv[0]
+  return parser
 
 def parseOptions(args):
-  try:
-    options, args = getopt.getopt(args, 'u:h:i')
-  except getopt.GetoptError, e:
-    print >>sys.stderr, e
-    usage()
-    sys.exit(1)
+  description = "Run a command on the given hosts or groups of hosts"

[Wladimir Palant, 2015/04/07 14:58:04]

Nit: single quotes, for consistency.

[mathias, 2015/04/07 15:36:51]

Done.

+  parser = createArgumentParser(description=description, add_help=False)
+  parser.add_argument(
+    '-i', action='store_true', dest='ignore_errors',
+    help='continue execution on next host in case of an error'
+  )
 
-  user = None
-  hosts = []
-  ignore_errors = False
-  for option, value in options:
-    if option == '-u':
-      user = value
-    elif option == '-h':
-      hosts.append(value)
-    elif option == '-i':
-      ignore_errors = True
+  hosts = set()
+  parser.add_argument(
+    '-h', metavar='<host|group>',
+    help='target host or group, can be specified multiple times',
+    type=lambda value: hosts.update((value,)) 

[Wladimir Palant, 2015/04/07 14:58:04]

Nit: I think this should be a list semantically - hosts.update([value])

[mathias, 2015/04/07 15:36:51]

Done.

+  )
 
-  return user, hosts, ignore_errors, args
+  parser.add_argument(
+    'args', metavar='command', type=str, nargs='+',
+    help='The command to run on the specified hosts'
+  )
 
+  options = parser.parse_args(args)
+  options.hosts = hosts
+  return options
 
 def getValidHosts():
   dirname = os.path.dirname(sys.argv[0])
@@ -87,11 +92,11 @@
     subprocess.check_call(command)
 
 if __name__ == "__main__":
-  user, hosts, ignore_errors, args = parseOptions(sys.argv[1:])
-  selectedHosts = resolveHostList(hosts)
+  options = parseOptions(sys.argv[1:])
+  selectedHosts = resolveHostList(options.hosts)
   if len(selectedHosts) == 0:
     print >>sys.stderr, 'No valid hosts or groups specified, nothing to do'
     sys.exit(0)
   for host in selectedHosts:
     print >>sys.stderr, 'Running on %s...' % host
-    runCommand(user, host, args, ignore_errors=ignore_errors)
+    runCommand(options.user, host, options.args, options.ignore_errors)