Ethan, John, thank you for the reviews! This bug fix has been pushed.
- Keith On 02/15/10 12:01 PM, John Fischer wrote: > Keith, > > The new webrev looks good. > > John > > On 02/12/10 09:51 AM, Ethan Quach wrote: >> 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 >> _______________________________________________ >> 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
