This looks good Clay! My comments below.
Joe Clay Baenziger wrote: > Hi all, > I've updated the usr/src/cmd/ai-webserver components to be 2.6 > clean. Can Jean and/or Joe look at these changes and get responses back > so that we can wrap up by the 9th? > Further, in testing, I found a number of bugs which hadn't been > addressed but which fully broke these components and prevented me from > testing my changes -- I've as such, fixed these too. Although this effort should focus on PY2.6 required changes, fixing bugs that prevented you from testing seems like the correct approach. > (I guess the > components don't see much use?) Unfortunately, there's not any > regression tests for publishing/listing/deleting manifests that I'm > aware of. As such I've done the following: > > Tests -- for manifest publication > (all should work unless otherwise specified): > ------------------------------------------------------------------------------- > > > Try the webserver stock out of an fresh create-service > Try to download the default.xml file via the BUI > (All tests below involve checking list-manifest and the webserver BUI to > ensure things are correct afterwords) > Try to publish a new default manifest > Try to publish a new default manifest (but with criteria) -- should fail > Try to publish a new non-default manifest (with one criteria) > Try to publish a the same non-default manifest (but with two criteria) > Try to publish a new non-default manifest (with any number of criteria) > Try to publish a non-default manifest (with a duplicate criteria set) -- > should fail > Try to download non-default manifest using manual testing interface > (http://<server>:<port>/manifest.html) > Try to remove an instance of the first manifest > (delete-manifest -i <instance> <manifest name> /var/ai/<port>) > Try to remove all instances of the first manifest > (installadm remove -n <service> -m <manifest>) After removing an instance or all instances did you try to add a new one? General Issue: I see a couple of PEP8 issues. Deferring PEP8 is the correct approach. However please file a bug to PEP8 this code. File: usr/src/cmd/ai-webserver/list-manifests.py Question 1: You use the strings multiple times, e.g.: "criteria" "instance" and "manifest". In "C" it would be better to #define a constant for these strings instead of having them in the code in multiple places. Is this also true in Python? Nit: This comment is confusing to me, especially the last line which doesn't seem to apply. 91 # add the necessary spacing to the header for the instance label (with 92 # space before to separate from manifest label) and 93 # space for excess instance numbers 94 # space before first criteria File: usr/src/cmd/ai-webserver/publish-manifest.py Question 1: Same issue as Q1 for list-manifests.py, e.g.: "unbounded" Would these be better as below? UNBOUNDED="unbounded" ... 209 if man_criterion[0] == UNBOUNDED: > > PyLint: > ------------------------------------------------------------------------------- > > > These files have seen some PEP8 clean-up, however, due to time > constraints not all PEP8 suggestions were done. The pylint scores using > the install.pylint rc file are as follows: > verifyXML.py > Your code has been rated at 5.84/10 > AI_database.py > Your code has been rated at 3.39/10 > publish-manifest.py > Your code has been rated at 3.12/10 > delete-manifest.py > Your code has been rated at 1.17/10 > webserver.py > Your code has been rated at 1.01/10 > list-manifest.py > PyLint fails to run on this > > Webrev: > http://cr.opensolaris.org/~clayb/python2.6/ > > Bugs addressed: > 8015 list-manifests output is hard to read > 11607 Should remove "prototype" out of AI commands > 12162 Need to transition the AI webserver to use the SQLite3 module > instead of PySQLite2 > 12424 AI Websever unable to handle manifest with null IPv4 or MAC address > criteria if set in at least one manifest > > Thank you, > Clay
