Thank you Clay! Looks OK to me.
Joe Clay Baenziger wrote: > Hi Joe, > Comment in-line. Thank you for the review! > Thank you, > Clay > > On Fri, 6 Nov 2009, Joseph J VLcek wrote: > >> 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? > > I've removed all instances in testing but not adding that as something > to test directly. > >> General Issue: >> >> I see a couple of PEP8 issues. Deferring PEP8 is the correct approach. >> However please file a bug to PEP8 this code. > > I've filed: > 12542 - Need to PEP8'ify /usr/src/lib/libaiscf_pymod code > 12543 - Need to PEP8'ify /usr/src/cmd/ai-webserver 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? > > Yes, we should have these abstracted out. Dave Marker is working on a > pretty awesome answer I think. It's a combination of XML and it's to > work for C and Python. I would prefer to go with an actual solution > rather than messing around with globals or something else ugly. > >> 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 > > Actually, the last one does apply (it's the '+ " "' on the end of line > 91 (in http://cr.opensolaris.org/~clayb/python2.6/webrev2/). However, > the part of the comment "(with\# space before to separate from manifest > label)" is bogus. Thank you for being confused by this! I've corrected > the bogus part. > > So what this is doing: > (for line 91 in webrev2's version) adding the header "instance" and > adding the spacing for the instance numbers defined by: > (max(len(_"instance"),maximum_instance_number_as_a_string) > > (for line 94 in webrev2's version) adding the necessary number of > hyphens to draw the separator out and then adding a space to separate > the column header (the '+ " "' at the end of the line). > > So the comment now reads: > # add the header for the instance label, space for excess instance > # numbers (instances whose numbers are longer than len(_"instance") > # and add a 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: > > Same answer as above > >>> >>> 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 >> >>
