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
>
>
