Hi Shawn,
On 12/12/12 10:18 AM, Shawn Walker wrote:
On 11/29/12 19:08, Tim Foster wrote:
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/
Thanks for the review! I'll respond to the rest of it on the original
thread (summary: all your points are reasonable, I'll work on fixing
them before mailing back)
In the meantime, I'd like to get more ideas for the name of the
executable/service, because that's something that's harder to fix after
putback.
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.
Right - the top-level "repos.shtml" page helps with some of that,
similar to the way we manage ipkg internally, but yes...
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.
Yep, I struggled with the name too. I initially called it apache-depot,
but was then nervous about
a) using the Apache name at all (for trademark reasons)
b) potentially tying us to a single webserver implementation (hence the
currently redundant -t flag)
pkg.depot-scalable doesn't seem right, because it's making a promise
about scalability, which while probably well-founded for most cases,
mightn't be true given enough clients. (it feels almost as bad as
calling something 'unbreakable' :)
pkg.depot-multi doesn't seem right either, but I can't explain why.
The current name pkg.depot-http also won't be right once we support
https, so I'm definitely happy to change it.
So with that in mind, can anyone suggest better names? I'm going to
suggest some here to get started.
In the below, I'm straying away from calling it a '<foo>d' since it
doesn't run a daemon per se. The executable builds an apache httpd.conf
file and produces a few static html files, that's all. The corresponding
SMF service starts Apache (so it does start a daemon)
pkg.depot-server
pkg.depot-store
pkg.depot-container
pkg.depot-collection
pkg.repository-group
pkg.repository-collection
pkg.repository-server
pkg.webserver
then the SMF service, called
'svc:/application/pkg/http-depot:default' in the webrev, would
correspond with the above, so:
svc:/application/pkg/depot-server
(too similar to svc:/application/pkg/server, I think)
svc:/application/pkg/depot-store
svc:/application/pkg/depot-container
svc:/application/pkg/depot-collection
svc:/application/pkg/repository-group
svc:/application/pkg/repository-collection
svc:/application/pkg/repository-server
svc:/application/pkg/webserver
Any more?
Of these, I quite like pkg.webserver and svc:/application/pkg/webserver
because it's short and descriptive, and while pkg.webserver _isn't
actually a webserver_, it's out of the way, being installed in /usr/lib,
and it ought to be obvious to any user that stumbles across it what it
actually does.
cheers,
tim
--------------------------------------------------
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
