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
