Looks ok to me too.
Sue
On 07/19/12 03:18 PM, John Fischer wrote:
Looks good to me.
John
On Jul 19, 2012, at 2:51 PM, Swati Sarraf wrote:
Hi John,
Thanks for the review.
updated webrev: https://cr.opensolaris.org/action/manage/caiman?__fsk=1948518347
-Thanks
Swati Sarraf
----- Original Message -----
From: [email protected]
To: [email protected]
Cc: [email protected]
Sent: Thursday, July 19, 2012 2:21:51 PM GMT -08:00 US/Canada Pacific
Subject: Re: [caiman-discuss] Code Review request: 7183018
Swati,
Looks good. Only a nit on the LOGGER.debug() when the image
size is unavailable. Instead of using the following text:
655 LOGGER.debug("Unable to get image size from image_info file, "
656 "so using fallback image size")
Can you use the text that is used by the text-installer?
LOGGER.debug("Unable to get image size")
If so then I do not need another webrev.
Thanks,
John
On 07/19/12 12:23 PM, Swati Sarraf wrote:
Hi All,
This is a stopper so it would be great if you can spare some time to review it.
-Thanks
Swati Sarraf
----- Original Message -----
From: [email protected]
To: [email protected]
Cc: [email protected]
Sent: Tuesday, July 17, 2012 2:41:29 PM GMT -08:00 US/Canada Pacific
Subject: Re: [caiman-discuss] Code Review request: 7183018
On 07/17/12 02:12 PM, Swati Sarraf wrote:
Hi,
Thanks for the review. please find the updated webrev:
https://cr.opensolaris.org/action/browse/caiman/ssarraf/7183018/
Regarding the indentation of line 655: I saw two different indentation method
in line 679 and line 665. which one is correct?
679 is better in this instance, since you have space on the line.
Sue
----- Original Message -----
From: [email protected]
To: [email protected]
Cc: [email protected]
Sent: Tuesday, July 17, 2012 1:45:06 PM GMT -08:00 US/Canada Pacific
Subject: Re: [caiman-discuss] Code Review request: 7183018
On 07/17/12 11:47 AM, Swati Sarraf wrote:
Hi All,
Can I have a code review for the following bug fix:
7183018 Gui livecd is not using size from image.info but using fallback image
size
http://monaco.us.oracle.com/detail.jsf?cr=7183018
Webrev link:
https://cr.opensolaris.org/action/browse/caiman/ssarraf/7183018/
Testing is done as follow:
1. Slim test done
Ran 1901 tests in 353.236s, FAILED (SKIP=1, failures=1)
failed test: Tests package with understood add option
Slim test pointer: file:///net/osol-bldx/datapool/ssarraf/7183018/
2. Pylint and pep8 test done , result OK
pylint result pointer: file:///net/osol-bldx/datapool/ssarraf/7183018/
3 Created DC image and tried installation with minimum size (4.7GB).
Installation finished successfully.
-Thanks
Swati Sarraf
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi Swati,
I had all the same comments as Karen, plus a few more nits:
651 No need for backslash.
652 Should be indented so that "Size.mb_units is lined up with str on line above
654
if its fail
->
if it fails
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
