Hi Shawn,
Thanks for the review, much appreciated.
On 05/14/12 08:18 AM, Shawn Walker wrote:
On 05/03/12 19:29, Tim Foster wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxy-v2
src/client.py:
line 3981: drop the parens
Fixed.
line 4011: s/with/with the/ (I think)
Yep, thanks.
line 4454: Instead of effective URI, I would suggest "LOCATION" since
'<system-repository>' is not really a URI. Likewise,
s/eff_uri/repo_loc/
Sure, changed.
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.
I had tried to modify the column width at 4564 to make everything fit,
but I'm going to revert to the current value from the gate because to
me, this is a losing battle - with 'non-sticky' and 'disabled' and any
sort of fully-qualified URL, we already blow 80 columns easily...
That said, I'm ok with using P even just to reduce the clutter. I'd
thought there was a locale library call for T/F or Y/N, but I think I'm
confusing it with nl_langinfo.Y_EXPR. I went with T/F because we use
true and false in pkg property output (though, we also have "Enabled:
Yes" in pkg publisher output but "E" and "D" seems silly here)
Ultimately, I hope that a follow-on fix to provide a -o option to pkg
publisher will help.
line 4643: shouldn't this be an 'else' case of 4649?
Sure.
line 4651: can '<system-repository>' be a constant?
Yes, I've changed that to SYSREPO_HIDDEN_URI.
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.
I've made this change. I had thought we discussed this before and
decided that it didn't warrant an API version change (perhaps I'm
confusing it with the conversation about ImageConfig versioning)
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).
Sounds reasonable, I've done this.
line 770: insert new comment line before? s/"// ?
Fixed.
line 771: s/In future/In the future/ ?
Fixed.
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.
Sounds reasonable. Yes, dropping the origin/mirror info is the right
thing to do. Doing more testing with older clients, deleting a proxied
origin using older bits was actually causing this assert to trip, so
I've changed this, thanks for spotting it.
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.
The old 'proxied-urls' property used by the system repository isn't used
anymore, see imageconfig.py line 914 and line 214 of p5s.py, which is
called by api_inst.write_syspub(..)
I've updated p5s.py to reflect that, along with a fix (and corresponding
t_sysrepo.py test) when writing the p5s file when multiple origins were
configured in the reference image with different proxies - since Apache
can only access a given URI using a single proxy, we must not have
duplicate URIs in the p5s file.
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
Yep.
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?
Yep.
line 395: missing '.' ?
Fixed.
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.
hmm...
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.
I disagree. It's not that the semantics have changed, it's that the
identifying features of an origin/mirror have changed.
I made the changes to __eq__, __ne__ and __cmp__ to ensure that when
removing an origin or mirror, you must specify the proxy and URI in
order to uniquely identify the RepositoryURI object we're removing (and
updated api_errors.UnknownRepositoryOrigin to account for proxy values)
Not specifying a --proxy flag implies a proxy value of 'None.
Before now, all we needed to identify an origin was its URI, because
that's the only property it had. (since ssl keys/certs are publisher
properties at present, rather than origin properties)
granular$ pkg set-publisher --no-refresh --proxy http://baz -g
http://foo solaris
granular$ pkg set-publisher --no-refresh --proxy http://zoo -g
http://foo solaris
granular$ pkg set-publisher -G http://foo solaris
pkg set-publisher: Unknown repository origin 'http://foo/'
granular$ pkg set-publisher -G http://foo --proxy http://noodles solaris
pkg set-publisher: Unknown repository origin 'http://foo/' with proxy
'http://noodles'
The intent of this changeset is to tie the proxy and URI closely
together because they both configure the transport-path to our repository.
Otherwise, when trying to preserve the old behaviour, it would be
impossible to remove just an unproxied URI and leave a proxied URI
configured, since with the configuration:
# pkg set-publisher -g http://foo solaris
# pkg set-publisher -g http://foo --proxy http://bar solaris
"pkg set-publisher -G http://foo solaris"
would remove both the proxied and non-proxied origins. The exception is
'-G *' which removes all origins.
line 1118: s/\.\././
Fixed (for modules/client/transport/engine.py)
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// ?
Fixed all of those, thanks. (787 was for modules/config.py)
src/modules/misc.py:
line 1413: can user or pass have '@' in them? if so, maybe
rsplit max 1?
They can, but when used in URLs, curl expects them to be escaped so we
never see them in URLs (you'd use '%40' instead of '@')
line 1423: so all parts have to have env variables to expand in
them or none?
Nearly, if there's two env variables separated by a ':', then we expand
both (but always both), otherwise if there's only a single variable and
no ':', then we just expand one assuming it expands to the form
"<user>:<password>".
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?
I haven't been able to find a precedent for this specific case (http
proxy authentication) but storing passwords in environment variables
isn't new. Samba has documented the use of $USER and $PASSWD environment
variables, for example.
We could insist on using simply hardcoding those environment names
instead, and assume that users won't have usernames or passwords that
match '$USER' and '$PASSWORD', narrowing the space for collisions
(though $USER being a default UNIX environment variable makes me nervous
of wanting to override that particular one) I've left this as is for
now because of that.
src/tests/cli/t_sysrepo.py:
line 554: need another newline
Fixed.
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?
How old? Going back to (say) 151a, and disabling a publisher on an
image of that era causes the image format to be bumped, so that becomes
a one way street.
In general though, I tried to be careful to preserve compatibility for
older clients (hence keeping the original 'origins' and 'mirrors'
property lists)
Using pkg bits just prior to this changeset with an image where we have
multiple instances of a given URI with different proxy values, the older
client collapses those multiple instances into a single unproxied
instance, so for:
granular$ pkg publisher
PUBLISHER TYPE STATUS P LOCATION
solaris origin online T http://foo/
solaris origin online T http://foo/
granular$ pkg publisher -F tsv
PUBLISHER STICKY SYSPUB ENABLED TYPE STATUS PROXY URI
solaris true false true origin online http://bar http://foo/
solaris true false true origin online http://baz http://foo/
going back to older pkg bits we see:
old$ pkg publisher
PUBLISHER TYPE STATUS URI
solaris origin online http://foo/
Changing publisher properties works,
old$ pkg set-publisher --no-refresh --non-sticky solaris
old$ pkg publisher
PUBLISHER TYPE STATUS URI
solaris (non-sticky) origin online http://foo/
and looking at the changes on these bits, we preserve the multiple URIs:
granular$ pkg publisher
PUBLISHER TYPE STATUS P LOCATION
solaris (non-sticky) origin online T http://foo/
solaris (non-sticky) origin online T http://foo/
There is one difference in the way we handle proxied images: since
'http://foo' doesn't exist in this image as an unproxied origin, we
can't delete it using the granular proxy bits:
granular$ pkg set-publisher -G http://foo solaris
pkg set-publisher: Unknown repository origin 'http://foo/'
whereas we can in the older client, which doesn't know about proxies:
old$ pkg set-publisher -G http://foo solaris
old$ pkg publisher
PUBLISHER TYPE STATUS URI
solaris (non-sticky)
which has the effect of deleting http://foo when flipping back to using
new bits:
granular$ pkg publisher
PUBLISHER TYPE STATUS P LOCATION
solaris (non-sticky)
granular$ pkg set-publisher --no-refresh -g http://foo solaris
granular$ pkg publisher
PUBLISHER TYPE STATUS P LOCATION
solaris (non-sticky) origin online F http://foo/
What happens when the packagemanager tries to manipulate origins/mirrors
that have granular proxies set?
Good question. Given a publisher with two origins with identical URIs,
one proxied, one not, it'll allow you to remove the unproxied origin,
but not the proxied one, since packagemanager doesn't ask for the
required proxy information to properly select the origin from the list.
Then, trying to add back the unproxied origin, it'll tell you the URI is
already in use.
Otherwise, PackageManager appears to work fine - it can install,
uninstall and list packages from proxied publishers.
Have RFEs been filed to have the packagemanager handle granular proxy
configuration?
No, I'll do that.
I didn't see anything in here to update .p5i to allow proxy to be
specified. How do you want to handle that?
I think we had this conversation before, I was initially thinking about
adding the proxy to the p5i format, but it was suggested that this could
be a bad idea:
http://mail.opensolaris.org/pipermail/pkg-discuss/2012-March/028807.html
suggesting that a server shouldn't be able to specify the proxy used for
its clients (unless I'm misunderstanding your question?)
Thanks again for the feedback, I've made the changes above and added one
additional test, to ensure that the system repository itself is actually
using the Apache ProxyRemote directive properly. There's a new webrev at:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxy-v3
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
