Hi Drew,
Thanks for the review and the interesting offline discussions that resulted. :)
On 05/ 3/12 01:11 PM, Drew Fisher wrote:
Sue,
image.py
--------
177-180: This seems really heavyweight for getting a new directory. I'm trying
to think if there's
any other way we could do this and I'm coming up empty. I assume that creating
the directory based
on timestamp won't work? If not, can you combine 177 and 178?
I agree it was a bit heavyweight - now using time.time() instead.
441: Change to
if fmri is None
ok
446: Change to
if publisher is None
ok
453: Can you try getattr?
if getattr(pkgfrmi, "publisher", None) != new_pub.prefix:
Tried it and it didn't work for the None case. As the alternatives produced code that was less
clear, you were ok to leave it as.
574: Change to
if specified_path is None:
ok
577-580: Can you change to:
return not os.path.exists(os.path.join(aismf.get_imagedir(), svc_name))
I don't prefer the one long line, but I'll change to:
return not os.path.exists(def_imagepath)
597: Change to
if image is None:
ok
651: NIT - align indent
After chatting, you said "nevermind".
set_service.py
------------
148: no need for ".keys()"
for clientid in clients:
agree, removed.
update_service.py
---------------
32: move line to 41
Leaving it where it is, per pep8
47-51: Why make this a function? Why not just set it on line 61
because it is called by installadm.py for installadm help
206-209: Can you change to:
return not bool(setsvc.do_update_basesvc(service, new_service.name))
(I'm not always sure how installadm handles returns from subcommands)
I'd prefer to leave returning the int since that will be the system return code
and the code is cleaner the way it is.
Thanks again,
Sue
On 5/3/12 11:43 AM, Sue Sohn wrote:
Can I get a code review of the changes for:
PSARC/2012/142 installadm update-service
7102885 update a pkg(5) based service
Webrev:
https://cr.opensolaris.org/action/browse/caiman/sohn/7102885/webrev.7102885
Tests:
Command line option validation and action (positive and negative)
Command restricted to pkg(5) based service aliases
Client installs of updated alias
Regression test of set-service -o aliasof and create-service
Ran unit tests (newly added tests pass and no new errors on existing tests).
In addition, QA has executed all test assertions from the test plan at:
http://onwiki.us.oracle.com/bin/view/SolarisQE/+ISIM+Improvements+Test+Plan
on both sparc and x86 AI servers and has run the installadm test suites (no
regressions).
Supporting requests filed:
o Test Suites:
QE Request: https://sqe-osso.us.oracle.com/sqept/view.php?ticket=29
o Install Guide:
7162893 Install Guide should be updated with new installadm update-service
subcommand, PSARC/2012/142
o installadm(1m) manpage:
7162895 update installadm(1m) man page to include new update-service
subcommand, PSARC/2012/142
Thanks,
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
