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?
[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
[snip]
line 3227: This is a change from the previous behaviour of only
affecting new publishers. I can't say with certainty which
behaviour is more desirable, but historically we intentionally
avoided updating search order for existing publishers during
an auto-configuration operation.
I don't think we were updating the existing publishers since we weren't
passing in search_* to the add_update_pub on line 3217. I've since
realized though that the code, as written, would not result in all new
publishers being added before all existing publishers. I'm working on
ensuring that's handled correctly.
line 3227: I'd probably add a comment here explaining that this is so
that when multiple publishers are part of a response when using -p
that all publishers after the first are put before all other
publishers.
Good idea.
[snip]
line 3473: this may no longer serve a purpose; I'd ask Danek if he
feels comfortable removing this entirely since this primarily
existed for scripters (such as zones). (The -P option to 'pkg
publisher'). If we can't remove it, we should at least discard
the man page documentation.
I don't have strong feelings one way or the other. I can imagine
scenarios where a user might just want the top ranked publisher as the
output of pkg publisher. I'll ask around.
old lines 4118-4121: This change was so that you could specify just a
publisher name to -p at image-create time?
No, if I remember correctly, that's so you can create an image with no
publishers configured.
src/gui/modules/webinstall.py:
line 287: not strictly your bug, but since you're changing this, can
you: s/ == "https"/ in pkg.client.publisher.SSL_SCHEMES/ ?
Sure
src/modules/client/actuator.py:
I'm ok with the changes, but what prompted them?
I needed smf methods in (what ended up being) two other modules. It
seemed like making a smf module to handle the interface made more sense
than having unrelated modules reaching into the actuator module or
duplicating the code.
src/modules/client/api.py:
lines 1529, 1821, 1834, 3911: s/set_repository/repository =/
line 3180: actually, if we're going to change this, it'd make more
sense to have them returned in search order; I think that's
reasonable given that the previous ordering was only lexical for
all publishers after the first. You don't have to do that, but
it would make the system a bit more consistent. A nice benefit of
that would be the elimination of the only calls we have to
api_inst.get_pub_search_order().
Sure, I'm happy to make that change.
line 3605: no docstring and sort of a weird function to expose to
general consumers; at the least, add a docstring, and remove the 0
from the name. I'd rather have version as a parameter for external
interfaces, and at current we don't need to expose version anyway.
Ok, I'll make the version a parameter and a doc string.
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?
[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.
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.
[snip]
lines 935-937: so we're asserting that a parent image won't have
publishers without origins, or that those will never be part of
the syspub configuration?
Publishers without origins should never be part of the syspub
configuration. I'll make sure that's the case. If there's a use case I'm
not thinking of, I can revisit this.
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.
[snip]
src/modules/client/transport/repo.py:
[snip]
lines 953, 1004: it's a bit weird to only add the proxy parameter for
this class and then turn around and assert that you can't specify
the parameter; the docstring needs to be updated at a min, but I
don't understand the change here
I've removed it from all __init__'s since we don't actually set the
proxy on a repo at creation.
[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?
line 1736: what about all the work you just did for base ports and
parallel test suite runs?
That hadn't landed before this went out for review. Once 18047 lands,
I'll pull our gate forward and merge, which includes using the base port.
I'll let Tim handle these (though I think he's already taken care of them).
src/util/apache2/sysrepo/sysrepo_httpd.conf.mako:
are all the template comments really needed?
src/util/apache2/sysrepo/sysrepo_publisher_response.mako:
lines 6, 24: you can drop these; in fact, you don't have to specify
anything which uses the default values for a new Publisher object
src/sysrepo.py:
line 102: s/"""/"""\/ will let you move the publisher bit down a line
line 107: it'd be useful for debugging to add 'pkg-server' to the end
of this and include pkg.VERSION there; just like pkg.depotd
lines 121-124: copy&paste?
line 405: you don't need this if you're passing ignore_errors=True
line 415: the the?
src/tests/cli/t_pkg_sysrepo:
line 948: s/environement/environment/
Brock
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
