Keith, ti_install.py. - 521
Just make sure "install_profile.log_final" is *not* an absolute path otherwise os.path.join() does something stupid IMO; it'll just take the second arg as the "joined" path. -ethan On 02/12/10 09:46, Keith Mitchell wrote: > Hi John, > > Thanks for the review! An updated webrev is here: > http://cr.opensolaris.org/~kemitche/14180b > > - Keith > > On 02/11/10 05:16 PM, John Fischer wrote: >> Keith, >> >> A couple of issues: >> >> line 194 -- raises ti_utils.InstallationErrorer >> probably should be ti_utils.InstallationError >> like others > > Indeed it should - good catch. > >> >> lines 482 - 487 -- uses a finally. Is there any possibility >> that an exception will be raised from the run_ICTs() >> and lost if finally is run? Or am I misunderstanding >> the Python 2.6 documentation (which very well could be the case)? > > If an exception is raised in the "try" clause, the "finally" block will > get run. After the finally block executes, the original exception will > "take over" again - it won't be lost. > >> >> line 515 -- why not use os.path.join() instead of using + >> to create a path >> >> >> Nits: >> >> The comments for the class and function introductions >> could be a little more verbose. > > I did a passthrough and updated some of the comments. > >> >> The comments at lines 493-496 do not seem to match the other >> function comments stylistically. >> >> 493 '''Do final cleanup to prep system for first boot, such as resetting >> 494 the ZFS dataset mountpoints >> 495 >> 496 ''' >> >> verses: >> >> 314 '''Call libtransfer to transfer the bits to the system via cpio.''' > > That's actually PEP8 compliant - multi-line docstrings have a blank line > in between the last line of the comment and the closing quotes, whereas > single-line docstrings are constrained to a single line. > >> >> >> Interesting/handy: >> >> " ".join(cmd) >> >> Thanks, >> >> John >> >> On 02/11/10 03:41 PM, Keith Mitchell wrote: >>> Hi all, >>> >>> I'd like to request a code review of my changes for bug 14180. >>> >>> Bugzilla link: http://defect.opensolaris.org/bz/show_bug.cgi?id=14180 >>> >>> webrev: http://cr.opensolaris.org/~kemitche/14180/ >>> >>> I tested the changes by using a modified keyboard_layout ICT that always >>> failed, and verified that the installer cleans up properly upon >>> completion. >>> >>> This webrev also includes a minor change to the README in the text >>> installer gate; this issue has no associated bugzilla bug, but I can >>> file one if needed. >>> >>> - Keith >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
