> 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.
> 
There used to be some reason that didn't work, but I can't think of why now, so I'll try it.
> > +    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.
> 
Ditto.
> > +    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.
> 
I'm interested in what everybody else thinks of this, since I find the repetition very unpleasant to look at.  Every time I see it, I think "we could just do that to a whole list of things and then add things to the list and I'll never make an error copying and pasting the code."
> > +        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)
> 
That seems reasonable.  I didn't do that initially because I didn't think we'd be using that function anywhere else, but having it is a good idea.
> >   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?
> 
Yeah, we can.  When this code was introduced, we didn't support a None timeout, so I didn't want to ship with a timeout turned on by default, but this is fixed now.
> > === 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 ...
> 
If we default them in the init, then a "None" coming from args could overwrite them.  This was a problem when we had multiple scripts, some of which had certain options and some of which didn't.  I think we should be able to set the defaults in argparse now, and we can also set them in the constructor so the class is still self-sufficient.
> > === 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
> 
I'll take care of that as well.
> > === 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?
I don't like explicitly checking for ProvisionedMachine, which is what we did before.  I don't think run.py should know or care about what Machine subclass it's using.  We could just do a try and catch the AttributeError, but that seems like it could open us up to other problems.