Drew,
Thanks for the comments...
On 08/06/10 09:12, Drew Fisher wrote:
On 08/05/10 16:07, Ethan Quach wrote:
Hi all,
The following is the code review for the AI manifest schema changes,
and the installadm criteria changes. It is a rather large review, so
partial/piecewise review would be also be fine, just let us know what
you're reviewing. We've pre-requested reviews from some of you
already, but all comments welcomed by Aug 16th.
Webrev:
-------------
http://cr.opensolaris.org/~equach/webrev.ai-schema/
Bugs:
--------
16423 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16423>
Updates to AI schema should be made
15449 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15449>
installadm add validates combined manifest against image-specific
schema as well as schema in /usr/share/auto_install/
6975043 <http://bugs.opensolaris.org/view_bug.do?bug_id=6975043>
separate criteria and ai manifest
thanks,
-ethan
Ethan,
I focused only on the python files.
*~*~*~*~*~*~
usr/src/cmd/ai-webserver/set_criteria.py
106-107 - we should be raising an exception (I would recommend
RuntimeError or SystemExit), instead of simply bailing out with sys.exit.
113-115 - same as above
I'll change both of these to SystemExit exceptions.
*~*~*~*~*~*~
usr/src/lib/install_utils/DefValProc.py
284-285: combine into one line
command_list = [XML_VALIDATOR]
OK.
*~*~*~*~*~*~
usr/src/cmd/ai-webserver/publish_manifest.py
97-98: change to:
parser.error([error_message])
the error() method automatically does the sys.exit(1) for you and
allows you to specify an error message to the user.
OK, will change that.
160: change to:
if not entries[0]:
to follow the syntax of truth testing in line 159
OK.
166: same as above
OK.
373: change to
if append_manifest is not None:
OK
705: the .keys() function call is no longer needed. you can do:
for name in criteria_dict:
and it will iterate over the keys
OK
722: the comment appears to be chopped off
I'll fix that. It should have said:
# If there was only a single value, means user specified
# this range criteria as a single value, add it as a single
# value
786: dead comment?
That line should be undented by four spaces. Its
describing the else clause.
1171-1172: indent each line 4 characters
Will do.
thanks,
-ethan
-----
-Drew
_______________________________________________
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
