Issue 2200 - PART I/II - Migrate to argparse

kick.py

Index: kick.py
===================================================================
--- a/kick.py
+++ b/kick.py
@@ -3,46 +3,39 @@
 
 import sys
 import getopt

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

getopt isn't being used any more.

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

Done.

-from run import resolveHostList, runCommand
-
-def usage():
-  print >>sys.stderr, '''
-Usage: %s [-u <user>] [-t|-q] [<host>|<group>] ...
-
-Runs provisioning on the given hosts or groups of hosts.
-
-Options:
-  -u <user>       User name to use with the SSH command (needs access to puppet
-                  master and all hosts)
-  -t              Dry-run mode, will produce the usual output but not change
-                  host configuration
-  -q              Quiet mode, suppress Puppet output to console
-''' % sys.argv[0]
+from run import resolveHostList, runCommand, createArgumentParser
 
 def parseOptions(args):
-  try:
-    options, args = getopt.getopt(args, 'u:vt')
-  except getopt.GetoptError, e:
-    print >>sys.stderr, e
-    usage()
+  description = 'Run provisioning on the given hosts or groups of hosts'
+  parser = createArgumentParser(description=description, add_help=False)

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

I don't think there is a point in reusing the parser creation, it merely
obscures the logic. Arguments for kick.py and run.py are just different, that's
due to the use cases being quite different. They should just create their own
argument parser each.

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

While I understand your point (and in fact had the same concern), I believe it
is actually quite useful: The new parameters (in the next patch-set) and their
handling won't be too trivial, and we of course do not want to repeat ourselves
here. Sure, there are other implementation options, but IMO this is the cleanest
one - and one of the shortest.

+  parser.add_argument(
+    '-t', action='store_true', dest='dry_run',
+    help='Dry-run mode, will not apply any host setup changes'
+  )
+
+  parser.add_argument(
+    '-q', action='store_true', dest='quiet',
+    help='Quiet mode, suppresses Puppet output to console'
+  )

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

If you are switching to argparse, then you should specify a longer version for
these arguments as well. Meaning supporting --test and --quiet.

Also a nit: the messages should probably start with a lower-case letter for
consistency (the default message for the help option does).

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

Done.

+
+  parser.add_argument(
+    'hosts', metavar='<host|group>', nargs='+',

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

Nit (here and elsewhere): we should use the usual argparse format for metavar -
just 'host|group' rather than '<host|group>'.

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

Done.

+    help='target host or group, can be specified multiple times',
+  )
+
+  options = parser.parse_args(args)
+
+  if options.quiet and options.dry_run:
+    print >>sys.stderr, 'Only one mode flag can be specified, either -t or -q'
     sys.exit(1)
+  elif options.quiet:
+    options.mode = ''
+  elif options.dry_run:
+    options.mode = ' --test --noop'
+  else:
+    options.mode = ' --test'
 
-  if set(('-t', '-q')).issubset(options):
-    print >>sys.stderr, 'Only one mode flag can be specified, either -t or -q'
-    usage()
-    sys.exit(1)
-
-  user = None
-  mode = ' --test'
-  for option, value in options:
-    if option == '-u':
-      user = value
-    elif option == '-q':
-      mode = ''
-    elif option == '-t':
-      mode = ' --test --noop'
-
-  return user, mode, args
+  return options
 
 def updateMaster(user):
   print 'Updating data on the puppet master...'
@@ -61,11 +54,11 @@
   runCommand(user, host, remoteCommand, ignore_errors=True)
 
 if __name__ == "__main__":
-  user, mode, args = parseOptions(sys.argv[1:])
-  needKicking = resolveHostList(args)
+  options = parseOptions(sys.argv[1:])
+  needKicking = resolveHostList(options.hosts)
   if len(needKicking) == 0:
     print >>sys.stderr, 'No valid hosts or groups specified, nothing to do'
     sys.exit(0)
-  updateMaster(user)
+  updateMaster(options.user)
   for host in needKicking:
-    updateClient(user, host, mode)
+    updateClient(options.user, host, options.mode)