Hi Darren,
Thanks for the review.
In image.py, you have:
391 + def accept_licenses(self):
392 + '''Accept licenses'''
354 393 plan = self.pkg_image.describe()
355 394 for pfmri, src, dest, accepted, displayed in
plan.get_licenses():
356 395 if not dest.must_accept:
357 396 continue
358 397 self.pkg_image.set_plan_license_status(pfmri,
dest.license,
359 398 accepted=True)
360 399
This should probably also look at the dest.must_display flag too, just in
case, and if it's true, then it should call set_plan_license_status with
the 'displayed=True' flag too.
I know it's unlikely the packages in here will have it, but it's always a
possibility it could occur and as such no harm to handle it I feel.
Will change.
In set_service.py, you have:
176 + # strip off leading '01'
177 + client = clientid[2:]
Is it guaranteed that the 01 will always be present here, or should we
check that it's there before we strip it?
It should always be there.
In update_service.py, do_update_service() there is the code:
142 print _("Copying image ...")
143 new_image = base_svc.image.copy(prefix=base_svc.name)
144 print _('Updating image ...')
145 update_needed = new_image.update(fmri=fmri,
146 publisher=options.publisher)
147 except (ValueError, img.ImageError,
...
161
162 if not update_needed:
163 # No update needed, remove copied image
164 new_image.delete()
165 print _("No updates are available.")
166 return 4
This seems expensive to first determine if there are any updates to be
made, could we possibly do the test for update first, then if there are
updates do the copy, etc.?
Doing that would involve modifying the image for the base service to
possibly change publishers and then change them back afterwards,
resetting the plan, etc. Plus, if an update *was* going to occur, the
image would then still need to be copied, publishers updated, and the
plan generated again, which is a lot of duplicate processing. Those
things, plus being able to copy the image and then cleanly and easily
removing it if there were any exceptions or problems (and not being left
with issues with the base service image) were big advantages and worth
the trade-off, in my opinion.
BTW, why do you need to create a mew service? Could you not update the
existing service in-place (locking it or something), or in a temporary copy
which you move back to replace the original? With this it looks like you
would end up with a new service with a service-name based from the image
details, and the old service you updated would be linked to this - seems
over-kill to create a new service all the time.
So potentially, if you updated 10 services you would end up with 10 new
services rather than the existing ones being updated.
Yes. However, an alias shares an image with another service. For the
following, if we update the default-i386 alias, we would not want to
update the image of the solaris11u1-i386-08 service.
Service Name Alias Of Status Arch Image Path
------------ -------- ------ ---- ----------
default-i386 s11u1-i386-08 on i386 /export/ai/s11u1-i386-08
s11u1-i386-08 - on i386 /export/ai/s11u1-i386-08
After installadm update-service default-i386:
Service Name Alias Of Status Arch Image Path
------------ -------- ------ ---- ----------
default-i386 s11_1-i386-13 on i386 /export/ai/s11_1-i386-13
s11_1-i386-13 - on i386 /export/ai/s11_1-i386-13
s11u1-i386-08 - on i386 /export/ai/s11u1-i386-08
Are there man-page updates for this - would be good to see how they read...
You can find them in the PSARC case referenced below.
Thanks,
Sue
Thanks,
Darren.
On 03/05/2012 18:43, 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
