On 11/29/12 19:08, Tim Foster wrote:
Hi there,
I've got a webrev of the http-depot work ready for review, if anyone has
bandwidth:
https://cr.opensolaris.org/action/browse/pkg/timf/http-depot-webrev-0/http-depot-webrev/
I always envisioned a 'depot' as a collection of one or more
repositories -- unfortunately, we ultimately decided having 'pkg.depotd'
serve more than one repository was likely to result in a very poor user
interface. Probably for the same reason you chose to have a separate
executable for this "new" service.
So I'm not in love with the new executable name; it implies that
/usr/lib/pkg.depotd doesn't support HTTP; yes, it's bikeshedding.
Perhaps 'pkg.depot-multi' or 'pkg.depot-scalable'? That at least seems
less misleading and makes it slightly more obvious what the difference is.
--------------------------------------------------
src/http-depot.py:
general: s/os.makedirs/misc.makedirs/ seems preferable, and that
should allow dropping the try/except for each case
general: All references to svc:/application/pkg/server should
probably be through a constant.
lines 35, 37, 47, 48: unused import
line 93: nit: make consistent with 83-90 (same multi-line style)
lines 105-108: unused? if so, don't need import on line 27
line 141: '-a' for "use SMF" seems an odd letter to use. Also, I'm
not claiming it's the best option, but did you consider making the
options more consistent with pkg.depotd to lessen potential confusion?
For example, -d instead of -R? We typically have only used -R for image
roots, whereas this is a repository root. I realise that perhaps we
shouldn't have done that, but we've already established a pattern of usage.
line 163: s/return a list of publishers available here/
the list of available publishers/
line 204: s/ie./i.e./
line 239: s/0755/misc.PKG_DIR_MODE/
line 247: When I did this in the past for pkg.depotd, Danek suggested
I shouldn't. (Enforce port range.) I'm inclined to agree as Apache
should already be handling this.
line 269-270: Can you elucidate a bit on this? Keep in mind that we
officially do not support the use of any unicode within the repository
for filenames / paths. The only part of the path that could be unicode
is the parent of the repository.
lines 301-303: This should really be:
with file(conf_path, "w") as conf_file:
conf_file.write(conf_text)
line 309: redundant
lines 319, 343, 353: the call to os.path.sep.join seems unnecessary;
os.path.join accepts multiple arguments
lines 322-329: more simply written as:
with file(os.path.join(versions_path, "index.html"), "w") as
versions_path:
versions_file.write(fragment and DEPOT_VERSIONS_STR or
DEPOT_FRAGMENT_VERSIONS_STR)
lines 346-349, 355-357: use with() pattern
line 375: extra newline
lines 398-399: Should it query all of the repositories and accumulate
the errors together instead of stopping on the first bad one?
line 434: s/Written/Wrote/ or s/Written/Created/ ?
line 442: The docstring "marked as pkg/standalone" implies something
different than 455 "standalone == False".
line 465: s/root, and prefixes supplied/root and supplied prefixes/
lines 473-474, 478-479: seems like these should be accumulated and
raised after processing all of them
line 486: prefixes also shouldn't contain unicode; I'm fairly certain
transport will explode in pretty colours if any of the URI contains
them. Also, aren't there other chars Apache doesn't like or that hold
special meaning? Such as '?' and '%' ? You could possibly just re-use
misc.valid_pub_prefix here to test for validity.
line 492: 0.0.0.0 may be more appropriate; it means INADDR_ANY, which
means all active interfaces. While 127.0.0.1 may not be externally
routable for the system or hostname resolution may not work properly for
it. See http://defect.opensolaris.org/bz/show_bug.cgi?id=8145#c4
--------------------------------------------------
src/man/pkg.http-depot.1m:
general: Sometimes there are two spaces after the end of sentences,
and sometimes not.
line 22: XXX?
line 45: We generally use ',' not ':' for multiple values. I suspect
you felt ':' was less likely in a path. Just a note on consistency.
line 89: s/ie./i.e./
line 100: s/to./on./ is the general nomenclature I believe
line 111: See earlier note about 0.0.0.0. I would also suggest to
the user that they use '::' if they want IPv6. I'd also check that
actually works :-)
line 155: The wording implies the default value is 'apache2' but
that is not explicitly specified.
line 166: s/for clients./for clients using an existing web service./
line 166: s/dropped into/placed in/ ?
line 179: The prefix prefix?
line 186: s/ directives/ directives/
lines 196, 202: XXX
line 256: I believe we now use 'Uncommitted'
--------------------------------------------------
src/modules/server/face.py:
lines 142-149: leftover debug code?
--------------------------------------------------
src/pkg/manifests/package%3Apkg%3Ahttp-depot.p5m:
line 27: s/http/HTTP/
line 27: s/server/package repository server/ ?
lines 58, 59: why 'renamenew' instead of 'true'?
--------------------------------------------------
src/svc/svc-pkg-depot:
general: lots of error messages aren't line wrapped;
also seems like error messages should be a constant
like we do in zone scripts instead of repeated
line 53: s/is is/it is/
line 77: surely there's a better option than eval?
line 114: s/ a //
lines 134-135: erp?
--------------------------------------------------
src/tests/cli/t_http-depot.py:
line 36: I think this is unnecessary due to line 35
lines 332-337: I think fmri.get_link_path() or fmri.get_url_path()
will do what you want for the most part
line 2623: the default sleeptime seems excessive given that's seconds
(~277 hours?)
--------------------------------------------------
src/util/apache2/depot/depot.conf.mako:
line 27: 'it be used' ?
--------------------------------------------------
src/util/apache2/depot/depot_index.py:
general: I see lots of places here that should really be using the
httplib constants instead of '503', etc.
lines 361-405: drop the 'else:' and de-indent since
everything above returns or raises an exception
line 597: This should be '(object)' instead of '()' at least.
Otherwise it's considered an "old-style" class I believe.
lines 679-680: The server should not expose general tracebacks.
(In short, don't send them to clients, just log them.) Also,
we historically avoided raising 500 errors as that could cause
Apache to stop proxying the server. I don't know it matters here
since this doesn't appear to be a reverseproxy scenario. The HTTP
spec says that servers are permitted to return a 404 error which
is my preference since it makes less obvious to would-be attackers
when they've potentially found an error case.
--------------------------------------------------
src/util/apache2/depot/repos.shtml:
line 46: s/following/following package/
line 48: s/the/the package/
line 48: s/on/from/ to be consistent?
line 53: If this page is accessible from multiple hostnames, things
could be confusing if the links here are absolute instead of relative.
Just be certain that the URI reflects the hostname things are being
accessed from. In the event that a server can be accessed from multiple
hostnames, the user probably configured it with an IP address (for
HTTP). Only in the case of HTTPS would the actual name matter (due to
the certificate, obviously).
--------------------------------------------------
src/web/en/base.shtml:
We could just kill the stats page. It's mostly useless anyway.
line 34: It'd be nice if we had something less ambiguous than
'http_depot' :-( Yay bikeshedding.
--------------------------------------------------
src/web/en/catalog.shtml:
lines 247-249: I think this can use get_url_path() too; might
allow simplifying this whole section
--------------------------------------------------
src/web/en/search.shtml:
lines 360-370: See note about catalog.shtml.
--------------------------------------------------
That's all I can think of for now.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
