Hi Sue,
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.
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?
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.?
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.
Are there man-page updates for this - would be good to see how they read...
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
