On 04/25/2013 12:32 PM, Max Brustkern wrote:

> https://code.launchpad.net/~nuclearbob/utah/consolidate-scripts/+merge/160963

> === modified file 'examples/run_utah_tests.py'

>   def get_parser():

looking these two snippets
1)
> +    parser.add_argument('-a', '--arch', metavar='ARCH',
> +                        choices=('i386', 'amd64', 'arm'),
> +                        help=('Architecture to use for VM creation '
> +                              '(%(choices)s)'))

2)
> +    if args.arch:
> +        arch = args.arch
> +    else:
> +        arch = config.arch

You could just set the default value in argparse to config.arch and save 
4 lines of code.

> +    if args.machinetype:
> +        machinetype = args.machinetype
> +    else:
>           machinetype = config.machinetype
> -    else:
> -        machinetype = args.machinetype

you could probably do the same with this as well by assigning the 
default in the argparse.

> +    else:
> +        kw = {'clean': (not args.no_destroy),
> +              'new': True,
> +              }
> +        for arg in ['arch',
> +                    'boot',
> +                    'debug',
> +                    'image',
> +                    'initrd',
> +                    'kernel',
> +                    'name',
> +                    'preseed',
> +                    'rewrite',
> +                    'series',
> +                    ]:
> +            kw[arg] = getattr(args, arg)

this can be done explicityline and save two lines of code doing 
something like:

  kw['arch'] = args.arch

and i think it might look more readable.

> +        kw['installtype'] = args.type
> +        if machinetype == 'physical':
> +            if arch and 'arm' in arch:
> +                inventory = ManualBaremetalSQLiteInventory(
> +                    db=os.path.join('~', '.utah-bamboofeeder-inventory'),
> +                    lockfile=os.path.join('~', '.utah-bamboofeeder-lock'))
> +                kw['machinetype'] = BambooFeederMachine
> +            else:
> +                inventory = ManualBaremetalSQLiteInventory()
> +                kw['machinetype'] = CobblerMachine
> +        else:
> +            inventory = TinySQLiteInventory()
> +            for arg in ['diskbus', 'emulator', 'xml']:
> +                kw[arg] = getattr(args, arg)
> +            kw['disksizes'] = args.gigabytes
> +        machine = inventory.request(**kw)

also - i think I'd break the whole block of "if args.skip_provisioning" 
to here. into a utility function called something like:

  def _get_machine(args)

>   if __name__ == '__main__':
>       try:
> -        try:
> -            if isinstance(config.jobtimeout, int):
> -                timeout(config.jobtimeout, run_utah_tests)
> -            else:
> -                run_utah_tests()
> -        except AttributeError:
> -            run_utah_tests()
> -        finally:
> -            cleanup.run()
> +        exitstatus = timeout(getattr(config, 'jobtimeout', None),
> +                             run_utah_tests)

why the "getattr" - can't we ensure this will always be available by 
providing a default in config.py?

> === modified file 'utah/provisioning/provisioning.py'

> -        self.debug = debug
> -        self.dlpercentincrement = dlpercentincrement
> -        self.machineid = machineid
> -        self.new = new
> -        self.prefix = prefix
> -        self.template = template
> +        args = ['arch',
> +                'boot',
> +                'clean',
> +                'debug',
> +                'dlpercentincrement',
> +                'installtype',
> +                'new',
> +                'rewrite',
> +                'series',
> +                'template',
> +                'uuid',
> +                ]
> +        for arg in args:
> +            setattr(self, arg, (locals()[arg] or getattr(config, arg)))

this pattern makes it hard for people to grep for how things get 
inialized. ie you can't do 'grep 'self.series =" *' to  learn things. 
And I'm not sure it saves that much code when you put them in separate 
lines. You could just as easily do something like:

   self.arch = (arch or config.arch)

Or better yet - why not default these values in the __init__ params ie:

instead of:
   def __init__(self, arch=None ...
do
   def __init__(self, arch=config.arch ...

> === modified file 'utah/provisioning/vm/vm.py'

> +        args = ['diskbus', 'disksizes', 'emulator', 'prefix']
> +        for arg in args:
> +            setattr(self, arg, (locals()[arg] or getattr(config, arg)))

same complaint

> === modified file 'utah/run.py'

> -        try:
> -            shutil.copyfile(machine.finalpreseed, p)
> -        except (IOError, shutil.Error) as err:
> -            logging.warning('Failed to copy preseed file: %s', err)
> +        if machine.hasattr('finalpreseed'):

do we really need hasattr instead of just doing machine.finalpreseed?