Hi Drew,
Many thanks for the review.
On 26/04/2011 23:26, Drew Fisher wrote:
> Darren and Matt,
>
> For the most part I only found nits. The target_selection checkpoint is
> cool. :) I didn't look at the test code, though...
>
>
> usr/src/cmd/auto-install/Makefile
> ---------------------------------
> 35-43: alphabetize
Done
>
> usr/src/cmd/auto-install/ai_manifest.xml
> ----------------------------------------
> 22: shouldn't the copyright date be 2008, 2011?
Yep
>
> usr/src/cmd/auto-install/svc/Makefile
> -------------------------------------
> It doesn't look like this file changed. If it hasn't can you revert
> it back?
Agreed.
>
> usr/src/cmd/auto-install/svc/auto-installer.xml
> -----------------------------------------------
> It doesn't look like this file changed. If it hasn't can you revert
> it back?
Agreed.
>
> usr/src/cmd/auto-install/utmpx.py
> ---------------------------------
> copyright isn't valid for a new file
Fixed.
>
> usr/src/cmd/installadm/__init__.py
> ----------------------------------
> copyright isn't valid for a new file
Fixed.
>
> usr/src/lib/install_common/__init__.py
> --------------------------------------
> 371: TODO comment?
Removed.
>
> usr/src/cmd/auto-install/auto_install.py
> ----------------------------------------
> 27-37: alphabetize imports
Done
>
> 43-65: alphabetize imports
Done
>
> 68: Why not inherit from object()?
Do now...
>
> 101: shouldn't debug be set to False?
Yep, it should be - logged a bug to track, since we like the exceptions for
the moment.
>
> 236-243: use with open(profile, "r") as fh: ....
Changed.
>
> 457-463: Can you move this check up higher so it's caught and
> executed sooner?
Looking into it.
>
> 568: TODO comment?
Possibly can remove it, will look into it.
>
> 581: I'm not the biggest fan on catching Exception. If you must do
> it at least capture the error string and log it somewhere.
>
> except Exception as err:
> self.logger.debug("Uncaught Exception in register_parse_manifest:
> %s" % str(err))
> return False
Done.
>
> (or something similar)
>
> 755: TODO?
Removed, I think it is an error if there is a destination specified.
>
> 781: s/Dump ADM/dumpadm
Done
>
> 771-815: why an 8 space indent?
No idea, now it's 4.
>
> 817: see line 581 above
Changed.
>
> 839: instantiate a dictionary with dict() instead of {}
>
Done.
> 997: use list() instead of []
Done.
>
> 1036-1042: use with() instead
Done.
>
> usr/src/cmd/auto-install/checkpoints/__init__.py
> ------------------------------------------------
> copyright isn't valid for a new file
Changed.
>
> usr/src/cmd/auto-install/checkpoints/target_selection.py
> --------------------------------------------------------
> copyright isn't valid for a new file
Changed.
>
> General Nit: Your docstring commenting style changes throughout the
> file. May want to standarize if you have time
Changed.
>
> General Nit: You use len(list) > 0 to determine if a list is populate
> at all. An empty list is boolean false, so you can simply change
>
> if len(list) > 0:
> to
> if list:
>
> and
>
> if len(list) == 0:
> to
> if not list:
>
Fixed.
>
> 81: TODO statement?
Removed not relevant any more.
>
> 210, 220: s/OptionS/Options
Fixed
>
> 452-457: Does this need to be updated with the latest change to the
> DTD for zpools in logical?
Removed comments, since not relevant any more.
>
> 480: the default for Zvol.action is "create". this can be deleted
> 481: add_zvol takes a 'use' argument. You can add it to 479 and
> remove this line
Changed to use this keyword.
>
> 493: same as 480
> 494: same as 481
>
> 786: s/amd/and
>
> 800-924: you're constantly checking the len(object.children) to see
> if the object has any at all. Shouldn't you be use object.has_children ?
Yep, should be.
>
> 849, 850: use disk_kid.action in ["create", "use_existing"]
Changed, and also used correct string use_existing_solaris2 :)
>
> 1068-1071: why not just do: tmp_device_map = device_map.values() ?
Good question :) Fixed.
>
> 1083, 1113: use device.has_children instead
Fixed
>
> 1116: odd 8 space indent
>
Fixed
> 1214, 1273: This isn't true. Slices need to start one cylinder into the
> Disk/Partition. Setting a start_sector of 1 will auto round to the
> first cylinder for you, however.
Still setting to 1, but commented to say TI will update to cylinder
>
> 1442-1446: move this above the partition walker and you can remove
> the if statement at 1419. If there are no partitions in
> partition_list, nothing gets done.
>
> something sort of like this:
>
> partitions = disk.get_children(class_type=Partition)
> slices = disk.get_children(class_type=Slice)
>
> <whole disk if-statement check>
>
> if self.arch == "x86" and not partitions and slices:
> raise SelctionError("Invalid specification of slices outside "
> "of partition on this platform")
>
> for partition in partitions:
> <stuff>
>
> <....>
>
Changed.
> 1559: get_children() returns a list, so you can do:
>
> if logicals:
> <stuff>
Changed.
>
> 1778: s/somehoe/somehow
>
>
Changed.
>
>
> Whew!
>
Agreed ;)
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
