Code review comment for lp:~nuclearbob/utah/consolidate-scripts

> 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.