Issue 2200 - PART I/II - Migrate to argparse

kick.py

 #!/usr/bin/env python
 # coding: utf-8
 
 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, createArgumentParser
 
 def parseOptions(args):
   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 = createArgumentParser(description=description)
   parser.add_argument(
-    '-t', action='store_true', dest='dry_run',
-    help='Dry-run mode, will not apply any host setup changes'
+    '-t', '--test', 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'
+    '-q', '--quiet', 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.

+    'hosts', metavar='host|group', nargs='+',
     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'
 
   return options
 
 def updateMaster(user):
   print 'Updating data on the puppet master...'
   remoteCommand = ' && '.join([
     'sudo hg pull -qu -R /etc/puppet/infrastructure',
     'sudo hg pull -qu -R /etc/puppet/infrastructure/modules/private',
     'sudo /etc/puppet/infrastructure/ensure_dependencies.py /etc/puppet/infrastructure',
   ])
-  runCommand(user, "puppetmaster.adblockplus.org", remoteCommand)
+  runCommand(user, 'puppetmaster.adblockplus.org', remoteCommand)
 
 def updateClient(user, host, mode):
   print 'Provisioning %s...' % host
   remoteCommand = 'sudo puppet agent%s' % mode
 
   # Have to ignore errors here, Puppet will return non-zero for successful runs
   runCommand(user, host, remoteCommand, ignore_errors=True)
 
-if __name__ == "__main__":
+if __name__ == '__main__':
   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(options.user)
   for host in needKicking:
     updateClient(options.user, host, options.mode)