On 05/03/12 19:29, Tim Foster wrote:
On 05/ 1/12 04:24 PM, Tim Foster wrote:
I've attached new sample output - if this looks good, let me know and
I'll post a webrev.
Ok, so here's the latest webrev for the granular proxy wad, which
addresses the comments Brock had made earlier, along with changing the
default output of "pkg publisher" to denote proxied/unproxied origins
and mirrors.
Along the way, I also picked up a small usage-message change, to
document -F for "pkg publisher"
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxy-v2
src/client.py:
line 3981: drop the parens
line 4011: s/with/with the/ (I think)
line 4454: Instead of effective URI, I would suggest "LOCATION" since
'<system-repository>' is not really a URI. Likewise,
s/eff_uri/repo_loc/
line 4460: Instead of "PROXIED", please use "P" or another char;
this particular command is already struggling to fit in 80 columns
most of the time. We already have precedent for single-column
output headers with commands like 'pkg list'. Just be certain
the new columns are documented in the man page (talk to Alta).
I also wonder if this should be 'Y' or 'N'; I'd ask Danek about
that. Might be simpler for translation purposes too.
line 4643: shouldn't this be an 'else' case of 4649?
line 4651: can '<system-repository>' be a constant?
src/modules/client/api.py:
This change needs an API version bump, even though it's compatible.
Please also update doc/client_api_versions.txt.
src/modules/client/imageconfig.py:
lines 624-630: Please don't use set() conversion to ensure
uniqueness; this could change the order in which origins and
mirrors are set causing subtle unnecessary differences in
configuration. While this currently doesn't matter for most
purposes, it can have subtle affects on how transport selects
between multiple sources. In addition, should we ever wish
the order to matter, this will make that impossible. Please
change this to eliminate duplicates manually and add a comment
explaining why a set is not used (so that order is preserved).
line 770: insert new comment line before? s/"// ?
line 771: s/In future/In the future/ ?
line 789-792: In general, I prefer to avoid asserts in the
configuration load path or fatal exceptions. Since if there's
anything wrong here, the client can no longer start and the
user is stuck with a broken image. I also believe that you
could get into this situation if you manipulated the image with
an older client. So I think the right answer here is to simply
drop origin/mirror info if you can't find the mirror or origin.
If that really bothers you, you could consider emitting a warning
using the logger object, but I'd avoid that as well personally.
lines 1038, 1042, 1060: is this really backwards compatible?
what happens if an older pkg version is used with a new image?
I'm envisioning backport issues here.
src/modules/client/publisher.py:
line 236: seems like this could be "if not" and then you could
move line 239-240 under that and then 241 handles both cases
line 285-286: this can easily end up with false negatives since
the URI is normalised after this point; i'm assuming that's
ok?
line 395: missing '.' ?
lines 613, 630, 658, 674, 723, 739, 767, 784:
These functions have had their operating semantics changed
incompatibly; previously, they would match solely on the URI.
Now objects have to match both on proxy and URI. That's not
right. If I call remove_origin() with a single URI, then it
should remove any origins with that URI regardless of proxy.
Look at __eq__ and __ne__ in RepositoryURI.
line 1118: s/\.\././
src/modules/client/transport/stats.py:
line 53: s/determine/get_stats/ since this isn't "determining"
or setting values; it's just getting a display value
line 503: s/a// ?
line 504: s/,// ?
line 787: s/ the// ?
src/modules/misc.py:
line 1413: can user or pass have '@' in them? if so, maybe
rsplit max 1?
line 1423: so all parts have to have env variables to expand in
them or none? What happens if the user has '$' in a user or
password? Is there an established precedent for this syntax?
Do any other utilities document this sort of expansion?
src/tests/cli/t_sysrepo.py:
line 554: need another newline
So what testing has been done to see what happens when you go back and
forth between using an older version of pkg and this version of pkg to
manipulate publishers?
What happens when the packagemanager tries to manipulate origins/mirrors
that have granular proxies set?
Have RFEs been filed to have the packagemanager handle granular proxy
configuration?
I didn't see anything in here to update .p5i to allow proxy to be
specified. How do you want to handle that?
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
