Hi Keith, Thank you so much for the code review. Please see my responses below:
On 01/14/10 16:00, Keith Mitchell wrote: > Hi Karen, > > pre_boot_archive_pkg_image_mod.py: > > Since this is an executable script, the entirety of the 'main' > function should be within a "if __name__ == '__main__':" clause. Yes, that's right. Will fix. > > install_utils.py: > > 695: The 'finally' clause will close this file before the return, so > this line isn't needed. (Did you consider using the "with" syntax for > opening files? I find it provides a cleaner way to manage > opening/closing files.) Since I am returning at line 696, I am not sure whether the finally clause will get executed. I know the finally clause will get executed if we continue with the rest of the function. I am not sure what will happen if I return. I do it here immediately before I return to make sure. So, you are sure that it will? If so, I can remove this. I am not very familiar with the "with" statement syntax. I will look it up and try to use it. > Some of the variable names used in loops (e, v, etc.) seem like they > don't meet our pylint requirements. I will change those to be longer variable names. I will post an updated webrev after I received and addressed comments from others. Thanks again for your review. --Karen > > The rest looks good to me. > > - Keith > > > Karen Tung wrote: >> Please review my changes to fix the following bugs: >> >> 12726 <http://defect.opensolaris.org/bz/show_bug.cgi?id=12726> DC and >> installers shouldn't require a root password in the packaged /etc/shadow >> 12669 <http://defect.opensolaris.org/bz/show_bug.cgi?id=12669> DC >> errors about /var/pkg/catalog >> 12186 <http://defect.opensolaris.org/bz/show_bug.cgi?id=12186> >> Deleting catalog cache needs to be revisited with with catalog v1 >> <http://defect.opensolaris.org/bz/show_bug.cgi?id=1> >> >> http://cr.opensolaris.org/~ktung/12726/ >> >> The following testing is done: >> >> - Build images using all modified manifests, name make sure that >> the install is successful for each image, and the installed system >> boots cleanly. >> Also make sure that I can use the default >> root password of opensolaris to login as root or assume the root role. >> >> - Built an image where I specify my own root password and make sure >> the installation is successful, and I can assume the root role >> using the password I provided. >> >> Thanks, >> >> --Karen >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
