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/
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.
Also, we'll need to install apache-22 on our hudson test box before this
is putback. Ping our hudson maintainer (ask off-list if you need his
name) about upgrading the hudson box (it's on b150).
src/client.py:
line 205: needs updating likely given my comments on 4118-4121 below
lines 226-242: needs updating for --search changes
line 2907: no longer needed
line 3038: s/_/-/; also, what about search-first?
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.
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.
line 3308: s/set_repository(repo)/repository = repo/; the function
has no value
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.
old lines 4118-4121: This change was so that you could specify just a
publisher name to -p at image-create time?
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/ ?
src/modules/client/actuator.py:
I'm ok with the changes, but what prompted them?
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().
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.
line 3616: this does mean that Ctrl+C won't remove the file; for
that you'd have to catch BaseException instead
src/modules/client/api_errors.py:
line 1650: missing docstring
src/modules/client/image.py:
line 659: what necessitated this change? If the ordering is
important, a comment needs to be added.
lines 2550-2557: this function isn't used anywhere I can find
src/modules/client/imageconfig.py:
line 192: although we screwed up in the past, it's preferable that
new properties use '-' instead of '_'
lines 756-767: need docstrings
line 769: need class docstring
line 817: maybe add a comment explaining why this is ok or should
be done
line 830: perhaps:
assert not p.disabled, "System publisher %s was unexpectedly "
"marked disabled in system configuration." % p.prefix
line 845: s/mergs/merges/
lines 845-864: seems like it should state what the return value is
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?
src/modules/client/publisher.py:
lines 292-299: why only 'http' and not 'https', 'ftp', or whatever
else we might support in the future?
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.
lines 1176-1181: just drop this function; callers can set this via
the property instead; no reason to have this really
line 1247: missing docstring; could also be a @property
lines 1670-1674: just drop this; callers can set .repository = None
src/modules/client/transport/repo.py:
line 307: s/:/: /
line 519: s/sysdepot_info/syspub_info/ ? that would fit better with
the other functions I think
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
src/modules/client/transport/transport.py:
line 2982: s/set_repository(repo)/repository = repo/
line 2995: not your bug, but would you mind:
s/origin.scheme == "https"/origin.scheme in
pkg.client.publisher.SSL_SCHEMES/ ?; been trying to
fix this up as I go along
src/modules/p5i.py:
line 137: s/set_repository(repo)/repository = repo/
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
src/tests/api/t_publisher.py:
line 356: s/in ("repository")/== "repository"/
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?
line 1736: what about all the work you just did for base ports and
parallel test suite runs?
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/
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
