I've got a webrev at:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies-4/
For convenience, I've also included an incremental webrev against the
previous granular-proxy bits I posted. It's not a perfect incremental
webrev, as it contains a few changesets that weren't in the previous
granular-proxy wad, but it may be helpful in terms of which parts of
this wad to concentrate on, given the previous reviews.
Couple of UI questions.
If I want to remove a proxy for an origin, how do I do that? For that
matter, how do I change what a proxy for an origin is?
I think we discussed this offline, but to confirm, this means I can't
have the same origin configured with and without a proxy, correct? This
means that an NGZ can no longer configure a direct connection to any
origin that the sysrepo is providing for it. Is that correct? I'm not
thrilled with losing this functionality but it seems like it was forced
upon you, and I'm not going to block the putback over that. I'd
appreciate an RFE filed (if there's not one already) to restore this
ability in the future.
client.py:
Some of this code looks like there can only be a single proxy per URI,
but other bits of code makes it look like we support multiple proxies.
Is the multiple proxy code just laying track for future work, or have I
not gotten my head wrapped around something?
api.py:
4558, 4563: You may want to sort, or otherwise order these so you don't
get spurious differences (unless the ordering of the proxies is significant)
imageconfig.py:
617-642: A comment here explaining that the reason we're duplicating
data (the uri's in both the key and the value) is to make inserting the
dictionaries into plist easier.
I think in general, I'm finding this code a bit confusing. Perhaps a
block comment describing what data structure we're creating and why we
want to create it? I'm struggling to understand why we're creating a
dictionary of lists of dictionaries. Especially when the lists of
dictionaries all get merged together at the end anyway. Why not just
build plist from the start? I see the comment on line 607, but that's
not covering *why* we need this 3 layers of structure.
898-899: Could you reformat these lines to make it clearer a list
comprehension is happening?
1223: How could we get here w/out proxy_url being set?
1231: I'm concerned about this line given that, generally, this code
looks like it supports multiple proxies (even if we're not exercising
that functionality yet). Specifically, I think we want to check whether
each proxy in the list is a "system" proxy, rather than just checking
the list contains exactly one item with the "system" proxy in it.
publisher.py:
line 88: Given bug 7140770, do we really support https proxies? (or are
you fixing that bug as part of the wad?)
line 147: indent looks too far
294: I feel like this should be comparing against self.__uri ... but I'm
not sure why I think that. So, take it or leave it as a suggestion.
301-304,314-319: Rather than doing isinstance(self,ProxyURI), why not
set "supported_schemes" (or pick a better name) as a class property
that's set to SUPPORTED_SCHEMES in RepositoryURI and
SUPPORTED_PROXY_SCHEMES in ProxyURI. Then you can just check if
self.__scheme not in self.supported_schemes. If the issue is with
raising the right exception, there's no reason that the exception
couldn't also be a class property (__unsupported_scheme_exception for
example).
343: Instead of have _override_uri, why not just store the uri in
self._uri, instead of self.__uri?
382: inserted extra white space
423: Why do we first set __system to None?
483: nit, """ at end of previous line please. 536, 568 too if it fits.
487: could you expand the comment to explain why? Also, it seems strange
(at least needing a comment) that we take proxy as an argument to init,
then just ignore it.
engine.py
138, 535, 1129: nit about """ again
127-131: This seems to be defined identically in many (every?)
subclasses ... why not just define it once here?
2267, 2285: typically there'd be a blank line between the docstring and
the first semantic line
stats.py
59: nit about """ again
91, 111: given that key() is being called below, I think these are now
TransportRepoUris, so the docstring should be updated. Ideally the
variable names would be changed as well to reflect that, but I don't
have to see that changed.
transport.py:
490: nit about """ again
485-506: I admit I'm not thrilled by this factoring of the code. I think
I'd be more comfortable with fromrepouri knowing what info a
TransportRepoURI cares about, and have it return a list of
TransportRepoURIs. This function could then just take care of flattening
the lists it got back into a single list. Is that a change that could be
made easily? I'm concerned that this isn't the obvious place to look
when we want to add more properties.
2831, 2846: could we have an assert of some kind to make sure that we're
only dealing w/ file repos in these methods?
misc.py:
2361+: If I'm reading this right, then if I set 'http_proxy' in my
environment, that's only going to be applied to uri's that are already
being proxied, just with a different proxy. Do I have that right? That
seems sorta busted to me...
t_pkg_sysrepo:
568-576: Not a big deal, but these might be easier to debug if they're
failing if these were assertEqual's to a specific line in the output,
which would also ensure the publishers were properly ordered.
t_sysrepo:
2: Just curious, where'd that come from? (It seems to be in a selection
of other files in the gate, so I guess it's not a big deal, but it's a
bit strange it's only in some of them...)
305: line wrap?
933: Why do another image create here?
Thanks,
Brock
Finally, please note that Danek really only had feedback about the data
model (that is, the structure of the objects internally); the specific
implementation suggestions here are largely my own. They likely need
further vetting before implementation.
No worries, they were definitely helpful. Along the way, there were
some subtle system publisher interactions involved that took some time
to get right, but the tests all pass now :-)
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
