On 01/20/12 15:06, Edward Pilatowicz wrote:
hey all,
could i get a review for:
https://cr.opensolaris.org/action/browse/pkg/edp/pkgplan_cleanup/
19137 package plan object should be easier to serialize
the changes should be pretty strait forward and boring. the main
interesting points are:
- stop caching "image", "check_cancelation", and "progtrack" within
pkgplan objects, instead we now pass pointer to these objects to
member functions that need them.
- switch from using pickle to json for pkgplan action serialization
- the license.py actions loses it's preinstall() callback since we
dropped filelist support ages ago.
- add a test case that verifies action serialization during staged
execution in child images.
i pulled these changes out of my parallel linked images gate in an
effort to reduce the delta in that gate and make it easier to review.
(it should also have the added benefit of making it easier for me to
sync up with changes in the pkg gate. ;)
thanks,
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
General comments:
Why all the renames from "img" to "image"? I'd prefer "img" since it
doesn't conflict with a module that we commonly import as "image".
I don't understand why "image" needed to be removed from
PkgPlan.__init__ nor why it's better to pass it in to a bunch of
functions instead of having it set once. Just because the "image" is set
in the PkgPlan doesn't mean it has to be part of what's serialized. To
load a PkgPlan, you'd create one with the image you want it to operate
on (or whatever image you're going to pass into evaluate or install or
...) and use setstate on it.
Since progtrack and check_cancel are only used in download (from what I
can see), I don't feel strongly about whether that should be treated
like "image" or only passed into the download function.
pkgplan:
111: Why is something being compared to the type of None? What else can
be in state[i] other than a string or None? Also, while it might be ok
in this one case, I'd prefer that isinstance was used instead of
comparing the types directly since if something was ever a subclass of
NoneType, an object of that type would fail this test (which sort of
brings me back to asking why the type is being used at all).
151: Similar question, why isn't this just 'if state[i] is not None:'?
What else can be in the origin or destination fmri other than None or a
PkgFmri?
159, 161: Same questions about using type(None).
pkg5unittest:
2800-2809: Shouldn't this also happen on update? I think it's probably
better located in _api_finish since update also shares this, and I think
potentially change-variant/facet could as well (though that would be one
weird package). It's probably also worth filing a test suite bug to
remind us that there are instances in the test suite where we used
self.pkg("... --accept ...") instead of the api because the api
interface was lacking and that those should be changed to use the api
interface now.
While the rest of the changes seem fine, I'd like to see another webrev
once image gets put back into PkgPlan as that caused a lot of noise
which made it hard to see the real changes here.
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
