Keith,
Thanks for the review. The delay is not a problem. I just appreciate the
work you guys are doing to help me get this in the release.
See responses below. I'll have an updated rev shortly.
Thanks,
John
On 12/21/10 04:42 PM, Keith Mitchell wrote:
Hi John,
Here is my review of the requested files. Sorry for the delay.
- Keith
Makefile.cmd:
Was there a reason to us var/ai/image-server rather than the existing
var/installadm?
Yes. var/ai/image-server gets created eventually and is where our
Apache webserver
files reside. I figured rather then having the script placed into
var/installadm that the
server stuff should all reside in a single location.
install-installadm.mf:
If you have the opportunity, use pkglint(1) to check this manifest.
Just ran it and everything checks out fine after I temporarily added
appropriate strings
for PKGVERS and ARCH.
test_ai_sd.py:
79/106: These should occur in setUp/tearDown methods, respectively (at
the class level). If a test assertion fails, line 106 won't get run
(in this case, no actual harm is done since bogosvc is in the local
scope, but as a general practice, it's better to perform all post-test
clean-up in a tearDown() method.
OK. I've added the following:
def setUp(self):
self.bogosvc = None
def tearDown(self):
if self.bobosvc:
del(self.bogosvc)
and changed the creation of bogosvc to be:
self.bogosvc = self.BogusService(self.SERVICE_NAME,
self.REGTYPE,
self.DOMAIN,
self.PORT,
self.TEXT_REC)
A test covering the failure case (where aisvc.lookup() returns -1)
should probably be added.
Added:
assert aisvc.lookup() != -1, \
'lookup did not succeed for service (%s)' %
self.SERVICE_NAME
ai_sd.py:
While looking at this file in relation to test_ai_sd.py, I notice the
copyright notice got incorrectly changed from "2008, 2010" to "2009,
2010". The first year should never change (though you may need to
change the second year to "2011" soon...)
Corrected.
test_cgi_get_manifest.py:
General: This is just a minor test readability nit, but grouping the
tests by function tested is preferred. i.e., group all the
get_parameters tests together, group all the get_environment tests
together, etc. (they can all be in a single class, or in separate
classes per function). By grouping this way, it's easier to evaluate
what unit test cases are covered on a per function basis. (Not all
functions, for example, need an "old version" test and a "new version"
test, based on what they do.
Updated.
227-231: A slight refactoring of cgi_get_manifest would allow for a
better injection of values here. My recommendation would be to modify
get_parameters() to accept a "FieldStorage" object as a parameter. The
main() function of cgi_get_manfiest.py could then call
get_parameter(cgi.FieldStorage()).
Changed.
278: In general, you should store the original value of sys.stdout and
restore that (sys.__stdout__ is generally usable for this purpose, but
discouraged. Consider for example if a unit test harness had
overridden sys.stdout - the wrong stdout would be getting restored
here). Also, this restoration should occur in a tearDown() method.
Corrected.
273: send_needed_criteria() could use tests for the error cases:
- Non-existent path based on the port
- Error occurs during lines 114-115 of cgi_get_manifest.py
Yes. I ran into some AI db issues. Still haven't worked around those yet.
347: send_manifest() could use tests for its error cases.
Yes. Same AI db issues.
list_manifest() may benefit from some additional cases, though that
one seems like a less-used interface?
Yes. It is only used by system administrators. It is like installadm list.
cgi_get_manifest.py:
While looking at this file in relation to the test classes, I noticed
that lines 216 and 221 might benefit from using
"some_string".partition() (rather than .split()). I'm also not
certain, but it seems like resetting "criteria = {}" on line 224 could
be a bug?
This particular part of the code is actually from the original cherrypy
webserver.py
code. It is unchanged. The lines following (226-229 and 242) handle
the case where
criteria is an empty dictionary:
# find the appropriate manifest
try:
manifest = AIdb.findManifest(criteria, aisql)
except Exception, err:
....
if str(manifest).isdigit() and manifest > 0:
On 12/13/10 04:46 PM, John Fischer wrote:
All,
Ethan found an issue with aimdns. Thanks Ethan!!
I have updated the webrev with the changes. The new webrev is at:
http://cr.opensolaris.org/~johnfisc/webserver-design/
http://cr.opensolaris.org/~johnfisc/webserver-design-diff/
http://cr.opensolaris.org/~johnfisc/webserver-design-orig/
Thanks,
John
On 12/13/10 10:06 AM, John Fischer wrote:
All (Ethan, Clay and Joe),
This is a request for a first review of the webserver design project.
I suspect that there will be at least 2 rounds if not more of review.
Please have feedback to me by Friday, December 17th, 2010.
The webrev is located at:
http://cr.opensolaris.org/~johnfisc/webserver-design/
This covers the design as specified in:
http://hub.opensolaris.org/bin/download/Project+caiman/auto_install/AI%2DWebserver%2DSpecification.pdf
I believe it covers the following CRs:
9538 AI webserver not restarting after server reboot
8282 Port sharing with multiple AI webservers
8708 SMF: ai-webserver not restarted if it crashes
8561 Blocker bug for AI Webserver design work
6047 HTTP_PORT should be obtained programmatically, rather than
defined
The design is fairly simple. The changes for the design include:
o replaces the cherrypy webserver with a cgi-bin script called
cgi_get_manifest.py.
- the scripts are very similar except without the cherrypy
specific code
o replacement of port reference for database with servicename
- an implication of the webserver consolidation, majority of
the important code
changes
o shell scripts no longer start/stop webserver
o compatibility for older services - configuration changes
o build changes - Makefile changes
There are a few additional changes from pep8 checks. I didn't
do the majority of the pylint changes because several of the files
require extensive changes.
Let me know if anyone needs a code walk through.
Thanks,
John
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
