On 04/ 6/11 12:03 AM, Brock Pytlik wrote:
On 04/ 4/11 02:25 PM, Shawn Walker wrote:
On 03/30/11 09:36 PM, Brock Pytlik wrote:
Here's the system repository work that Tim and I have been working on
for quite a while now.
http://cr.opensolaris.org/~bpytlik/ips-sysrepo-v1/
Thanks for the fast review Shawn. Comments are inline.
general comments: has onu been tested with these changes? There's
likely going to need to be some sort of coordination with whoever
maintains onu and at the very least a flag day when these changes go
into a build.
I haven't tested with onu. I'm happy to do it but can you explain what
concerns you have so I make sure I test the right things?
onu has some special logic for upgrading zones that may break.
There may have also been a subtle change in behaviour here that onu was
depending on, so testing with it doesn't hurt.
[snip]
src/client.py:
line 205: needs updating likely given my comments on 4118-4121 below
lines 226-242: needs updating for --search changes
I've added --search-first, I think that's all the needs changing for
set-publisher
It seems like -P should be removed from the man page if we've added
--search-first. I'd ask Danek.
...
line 3616: this does mean that Ctrl+C won't remove the file; for
that you'd have to catch BaseException instead
A bare except works right?
Yes, although it requires some care to ensure the exception is properly
re-raised if you want to raise it. See the bare except clause in
pkg.client.imageplan in execute().
[snip]
src/modules/client/image.py:
line 659: what necessitated this change? If the ordering is
important, a comment needs to be added.
It's there because otherwise catalog retrieval for the system publishers
fails.
I'll add a comment but I'll note that if we need to comment every
ordering constraint for every line of code, we're going to have a rather
large number of comments.
I don't see how that's possible. No catalog retrieval should be
happening at this point yet. In particular, the format of the image
hasn't even yet been established so it's not really safe to be doing so.
I think this issue needs a bit more exploration, or I need to better
understand the challenge you encountered here.
lines 2550-2557: this function isn't used anywhere I can find
Ok, guess I'll nuke it then.
src/modules/client/imageconfig.py:
[snip]
line 817: maybe add a comment explaining why this is ok or should
be done
Would you prefer I raise an exception if we can't get information from
the smf service? Trying on the default seemed better to me but I'm with
with just erroring out.
My concern was that we might never work right in this particular case
and wouldn't know it. I leave what to do up to you, although if you
leave it as is, I'd appreciate a comment at the least.
...
src/modules/client/publisher.py:
lines 292-299: why only 'http' and not 'https', 'ftp', or whatever
else we might support in the future?
changed to "if scheme != 'file'"
line 835: since this is a publisher object, the '_pub' as part of
the name seems redundant; why not just 'system' instead? Nitty,
I know.
Well, it does prevent mental collisions with the sys module.
I'm not sure I follow that logic, but however.
...
[snip]
src/setup.py:
line 218: s/'sysrepo'/'pkg.sysrepo'/; let's make it obvious it belongs
to pkg(5) and isn't something more generic
I think Tim's fixed this
[snip]
src/tests/pkg5unittest.py:
line 1473: it was intentional that you could specify a repourl and no
prefix; that's a valid input for image-create; why this change?
Really? It looks to me like before, if test was None, you'd get a
command line of:
'pkg image-create -F -p None=<repourl> ...
If it prefix could be None, I don't think that was actually what we
wanted. Perhaps I've misunderstood what you meant though, can you clarify?
Your change says 'not prefix' which is very different from 'prefix is
None'. There were (or are) some unit tests that set prefix="" here.
I'm surprised they didn't break with this change, but I'd prefer this
say 'prefix is None'.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
