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?
install-installadm.mf:
If you have the opportunity, use pkglint(1) to check this manifest.
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.
A test covering the failure case (where aisvc.lookup() returns -1)
should probably be added.
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...)
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.
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()).
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.
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
347: send_manifest() could use tests for its error cases.
list_manifest() may benefit from some additional cases, though that one
seems like a less-used interface?
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?
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
