Hi Jean, All your responses are OK with me. Regarding the one about setting the alternate authority failed, and yet, you still do the unset, can you add a comment there to explain why you have to do that, so, anybody else reading the code in the future won't be confused like me?
After adding the comment, please push anytime you like. I don't need to review it again. Thanks, --Karen Jean McCormack wrote: > Karen Tung wrote: > >> Hi Jean, >> >> Here are my comments: >> >> transfer_mod.py: >> >> - line 1074-1075: in all other ips calls, when the command fail, you >> do raise TAbort(), why not >> do that her? >> > Changed > > >> DC_tm.py: >> >> - DC_ips_set_auth() function: why don't you check to make sure >> pref_flag and mirr_flag are not >> specified at the same time right here? I know you check in >> transfer_mod.py too, but then, failing >> sooner is better. >> > Sure. Why not. I put a check in. > >> - line 207: Nit: It would be less confusing to the reader of the code >> to name it "mirror_url" intead >> of "pkg_url". So, that would make the statement to be "for mirror_url >> in mirror_url_list" >> > Agreed. Done > >> - lines 233-236: should we log this regardless quit_on_pkg_failure is >> specified or not? >> > Yes. Done. > >> - line 240: if the setting of alternate authority failed, that means >> we can't set the alternate authority, >> why do we need to unset it here? >> > When testing, I found that sometimes it ends up failing late enough that > it's still listed. So I put in the unset. > >> - line 257-260, 235-356, 377-381, 388: all these should be logged >> regardless quit_on_pkg_failure is true or not. >> > Agreed. Done. > >> - line 360: again, I don't quite understand why we need to unset the >> auth if we can't successfully set them in the first place. >> I am sure I misunderstand something, please explain. >> > See above. > > Jean > >> Jean McCormack wrote: >> >>> Can Karen please review this? Others are welcome to review this if >>> they wish. Please let me know as I'd >>> like to push this tonight or tomorrow morning. >>> >>> >>> Defects: >>> >>> 3358 Cleanup repo mirroring items in manifest >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3358 >>> >>> >>> 3360 Allowing specifying the default repo and default authority >>> installing pkgs >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3360 >>> >>> 3668 constructor should use pkg purge-history upon completion >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3668 >>> >>> >>> Webrev: >>> >>> http://cr.opensolaris.org/~jeanm/DC_pkg_bugs/ >>> >>> thanks, >>> >>> Jean >>> _______________________________________________ >>> 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 >
