Thank you for the review.
On Jul 15, 2011, at 8:34 AM, William Schumann wrote:
> Kristina,
> __init__.py
> OK
> common.py:
> OK
> conv.py:
> 479: duplice -> duplicate
Fixed
> 805: is the boolean sense reversed?
Missing a not. Updated tests to reflect that condition not being caught
> 925: more readable, order of evaluation more obvious: not size in -> size not
> in
Fixed
> 1116: present -> presence
Fixed
> 1121: if device is ever None, startswith is not a method for NoneType and
> will throw exception
The device should never been None but added if not None
> 1141: not sure what purpose of the period is here, but it appears to be
> dropped on first glance
Jumpstart profiles may have code that looks like
filesys rootdisk.s0 4000 /
That code simply replaces the "rootdisk." with the actual rootdisk device so
this gets translated to
filesys c0t0d0s0 4000 /
I added an example to the comments to help make this clearer what's going on
> 1274: comment repeated
Removed
> 1279: preceeded->preceded (proceed/proceeded, precede/preceded)
> 1281: created like->created, such as (good message, though)
Changed to
self.logger.error(_("%(file)s: line %(lineno)d: swap "
"mount is only supported when preceded "
"by a entry that causes the root pool "
"to be created. For example root_device, "
"boot_device, pool, or "
"filesys with a mount point of '/'") % \
{"file": self.profile_name, \
"lineno": line_num})
> 1547: device1 was removed
Fixed
> 1568: "any" -> DEVICE_ANY
> s/outputed/output globally
Fixed. That was the only none comment one
> 1890, 1899, 1908, 1910, elsewhere: why are blank lines before the end of """
> comments?
When my 1st review was done for this project, Keith commented that I need to
follow this format.
It had something to do with the way emacs handles comments. Hence all my
comments follow
this pattern now.
> 1887, 1896: use 'pop' instead of 'fetch', if item is removed from stack
Fixed. Name of routines changed to pop_xxx
> 2326: code has been commented
Removed
> 2332: precedence (not yours)
> In several instances, if 'len(x) == 0' has been replaced with 'if not
> len(x)'. 'not len(x)' mixes types inappropriately, since len() returns an
> integer, not a boolean. Why were these changed?
> 2399 remove backslash
Fixed
>
> tests look OK
>
> William
> On 07/13/11 09:33 PM, Kristina Tripp wrote:
>>
>> Hi,
>>
>> Could I please get a code review for the following js2ai changes
>>
>> 7062927 js2ai need to delete slices prior to create when user specified
>> whole disk
>> 7063455 js2ai add warning error if hostname and/or root password is not
>> specified in sysidcfg
>> 7066575 filesys update support for swap
>> 7066626 Expand support for usedisk
>> 7066637 js2ai: error: local variable 'processed_data' referenced before
>> assignment.
>>
>> https://cr.opensolaris.org/action/browse/caiman/enpointe/CR_7062927/webrev/
>>
>> These changes are primarily to address issues discovered when doing live
>> testing last week.
>>
>> As part of filesys swap support work the main __convert_filesys_entry
>> routine was broken out into different routines for handling swap and /
>> filesys entries. This does result in some duplicate logic
>> __convert_filesys_mirror_entry and __convert_filesys_entry But the logic is
>> now much easier to follow and maintain now.
>>
>>
>> Kristina Tripp, Senior Software Engineer
>> Oracle Revenue Product Engineering
>> 500 Eldorado Blvd, MS UBRM05-171
>> Broomfield, CO, 80021
>> Office: 303-272-8655
>> Email: [email protected]
>>
>> Oracle is committed to developing practices and products that help protect
>> the environment
>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> [email protected]
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Kristina Tripp, Senior Software Engineer
Oracle Revenue Product Engineering
500 Eldorado Blvd, MS UBRM05-171
Broomfield, CO, 80021
Office: 303-272-8655
Email: [email protected]
Oracle is committed to developing practices and products that help protect the
environment
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
