On 08/05/11 16:32, Edward Pilatowicz wrote:
On Tue, Aug 02, 2011 at 06:37:22PM -0700, Brock Pytlik wrote:
On 07/22/11 14:25, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v1
New webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v2
sorry for the delay.
i still need to review the tests.
comments on the code are below.
ed
----------
general
- i thought that the last time we talked about this we had agreed on
putting a copy of the "parsable_version" into the progress tracker
class? if we did this we wouldn't have to add an extra parameter to
all the linked image functions, and we wouldn't have to scan the
arguments in __pkg_cmd().
I don't remember agreeing on that, I remember discussing it. When I got
back and looked at the code more, I decided that I didn't like it inside
the progress tracker since it doesn't have anything to do w/ the
ProgressTracker, and just stuffing it inside didn't strike me as right.
----------
src/modules/client/api.py
- in __plan_op() this comment seems wrong:
Eventually these will yield plan descriptions objects as well.
i thought eventually we'd yield plan descriptions objects instead.
You're right. I intended to replace "as well" with "instead".
- in __plan_op() we should not plan all children and then return the
results. instead do_recurse() should yield results as they become
available.
If you can make that work, you're welcome to tell me how. I spent the
better part of an afternoon trying to convince it to do what we wanted
it to do without rewriting the entire stack of code. Since all the
parsable output has to be gathered up together in any case, I decided to
punt on the issue.
- all the gen_plan_*() functions have the following comments, which are
now wrong:
This is a generator function that returns PlanDescription objects
Ok, I'll change/remove the comment.
- i don't think we should bother with parsable output mode for
detach_linked_children() since that will never change any packages.
(it shouldn't even be going through the solver, see bug 18552.)
Ok.
- the detach_linked_children() docstring describes the tuple returned by
linked image operations. please update this for the new value your
adding.
Sounds like I won't need to do this given the comment above.
- instead of adding the None p_dict value to the return code in
audit_linked() could you add it in audit_self()? (just for
consistency so that all the LI interfaces return the same tuple.)
I'll give it a try.
----------
src/modules/client/imageplan.py
- should mediators() be __mediators()? (is it accessed outside of the
class?)
Nope (for the reasons Shawn said)
----------
src/modules/client/linkedimage/common.py
- in __rvdict2rv(), when you merge all the p_dict values into a p_dicts
list why not drop any None values?
Why should we?
- iirc you said that if there were any error you don't want to return
any parsable results. if that's the case thein in __rvdict2rv()
perhaps we should do:
err = apx.LinkedImageException(bundle=errs)
p_dicts = []<--- add this line
Actually, what I meant to say was that if there's an error, we don't
want to print parsable output to the screen. I think that emptying
p_dicts is overkill from the point of the consumer, but if I'm fine w/
it if that's how you'd like it.
- i'm not sure how the python subrpocess module deals with pipes and
signals. have you tested what happens if you kill a child process
while the parent attempting to read from the stdout pipe? (this can
generate a signal in the parent and unless python catches it and
handles it you might want to use a temporary file instead of pipe.)
I'll check and if necessary add something to handle the pipe exceptions.
----------
src/client.py
- why is __display_parsable_plan() part of the cli client? shouldn't it
be part of the PlanDescription class itself? having the
PlanDescription output format be part of the client seems weird.
other API consumers might want to output this format, etc. also,
presumably we might eventually want to have the plan description
reload a parsable plan so a python consumer could manipulate it
programatically?
It's part of the cli client because that's where the rest of our display
logic is, and this ISN'T a general purpose parsable output, it's being
made for one specific consumer.
If/when we decide we want to do something more general, we can rearrange
the code as desired.
- in __display_parsable_plan(), why pass in an api_inst instead of just
passing in the plan for the current image?
Because, among other things, we may need to retrieve the license text.
- in __display_parsable_plan(), this seems weird:
space_available = child_images[0]["space-available"]
if there's no plan for the parent, why not just default this to 0 vs
what is essentially a random value?
Sure.
- in display_plan(), if we're in parsable output mode we don't display
any licenses or mark them as installed. is this correct? if so it
seems like parsable output mode can't actually be used to install
packages? if so, perhaps -n should be required for parsable output
mode?
No. We include the licenses and text in the parsable output.
- in __api_op(), please add "parsable_version" to the kwargs array at
the same place where we add "noexecute" and other options.
Fine.
- in __api_op(), in the first call to display_plan, child_plans is
always an empty list, so why not just specify [] (or assert that it's
an empty list).
Fine.
- you duplicate the "try: __display_parsable_plan(...) except ..." code
block in lots of places. how about a function?
I don't really think that'd be helpful in this case since I'd still need
to check the return value.
Thanks for taking a look.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
