Keith,
Thanks for reviewing.
This response addresses the comments for the files I worked on:
auto_ddu_lib.c:
auto_install.c:
auto_parse.c:
auto-installer:
On 08/09/10 20:11, Keith Mitchell wrote:
Hi Ethan,
I focused on the Python files, but scanned the others as well. Please
forgive any duplicated comments; I haven't had time to read through
the other reviewers' responses - just point me to your response to
them as needed.
- Keith
AI_database.py:
298-300, 333-334: This seems like a rather narrow, single-purposed
addition to the function - perhaps it could be excludeManifests and
accept an iterable / list?
334: Nit: Use +=
publish_manifest.py:
62-74: Nit: The indentation on line 65 is inconsistent with lines 68,
71, and 74
78-81: Nit: can just do "options = parser.parse_args(cmd_options)" -
if it's None, OptionParser will handle it as if nothing was passed in.
92-93, 97-98: Just use "parser.error()" - it automatically prints the
usage and then exits.
122: Nit: Can we change this comment to
"aiwebserver=example.com:46503"? Using our own system names seems odd,
especially since anyone can open up this file on their system and look
at it. I saw a "suexml" somewhere in a test file, as well...
126: Pre-existing, but this error message doesn't tell the user how to
fix it. Should they delete the service? Manually add the needed SMF
prop? (Sorry, my eye wanders during reviews).
138-142: PEP-8 violations: No spaces around "=" in keyword arguments.
831-833: Ditto.
146: Would have been nice to see this implemented as a callback[1]
function in OptionParser. Nevertheless, since it's functionality is
strictly for option parsing, the use of SystemExits for errors is
acceptable, but please add a note to the docstring stating that.
(Alternatively, raise ValueError, and for current use cases, catch
"ValueError as err" and then run "parser.error(err)")
157: As I recall from some conversations with Sue, isn't one of the
values for criteria a filename? If so, isn't the lower() call going to
have an adverse affect for files that have capitals? I can understand
calling "lower()" on all the keys at this stage, but lowercasing the
values at this stage seems unnecessary.
160: Unnecessary check - OptionParser will ensure that options that
require arguments have them.
171: Nit: More informative message of the form "Criteria must be of
the form <criteria>=<value>" would be preferred.
302-304: Inconsistent indent (tabs?)
586-587, 1473: Nit: "+" operator not needed. I wouldn't mention it
except that I know that the python interpreter, when reading files in,
interprets strings differently if they're explicitly concatenated with
"+" - this affects string formatting operations, and I'm unsure if it
also affects gettext. I do expect that it's slightly more performant
to skip the "+", since based on those observations, I expect
implicitly concatenated, hard-coded strings are concatenated just once
(when compiling the .pyc file), whereas strings concatenated using a
"+" operator are probably concatenated at runtime.
(For all that it took to explain, it's still just a nit...)
597: I'd like to see us moving away from using SystemExit as our
'default' exception for anything other than OptionParser errors and
other errors in the highest level of the application (i.e., the "if
__name__ == __main__" block or a "main()" function), as its usage
limits the extensibility / modularity of a function.
Nit: Need a space between % and err at the end. Also happens on line 646.
631, 696: "open()" is preferred to "file()"
634-637: Unneeded. Let 643-644 handle it.
655, 666: Again, something other than SystemExit please - particularly
for a new function where there's no existing usage.
Assuming criteria_dict is from the criteria_to_dict() above:
684: This comment is incorrect
705: This could be "for name, value in criteria_dict.iteritems():"
708-709: These lines are possibly redundant with the check in
criteria_to_dict(). Given the name of this function, it might be best
to move all *validation* from criteria_to_dict into this function,
unless this function is run much later in execution and having the
first level of validation allows short-circuiting of basic typos.
723: Statement would be more obvious if it were "if len(range_value)
== 1:"
749: Given the functions defined here, it seems like it'd be more
appropriate to subclass dict.
760: Nit: Just say "__getitem__ = get_criterion"
1050: Might be best to not define these as keyword args
1068-1071: Checks here are probably unnecessary if previous comment is
followed.
General:
The four lines:
try:
some_val = self._criteria_root.iterfind(".//something")
except lxml.etree.LxmlError, err:
raise SystemExit(_("Error:\tmanifest error: %s) % err)
is repeated so often I question whether a helper function would be
useful. Although, going back to my comments about SystemExit, it can
probably just be left uncaught, and let the high-level application
exit code just print the error...
1450-1451: Nit: "self._criteriaroot.getroot().extend(ai_criteria)"
should work
set_criteria.py:
54: Need a space after the colon in "append:" for this to show up
right when printed.
58: ditto
72: See prior nit on not needing this if/else clause.
72-75: Though you don't need the args, perhaps you should check to see
if the user specified any, and error out if so?
80, 87: Use parser.error()
92: Would prefer to see this function return True/False, and have the
caller exit appropriately. (See above on SystemExit)
106: Is this expected to happen much/ever for an end-user? If so, does
"DB" mean anything to an installadm user? How would they resolve this?
114: I'd print the full path queried so the user knows exactly where
to look if needed.
117: Using both "nvpairs" and "nvpair" as identifiers is REALLY hard
to read. I initially was confused into thinking there were some major
bugs going on here. Suggest renaming one or both.
132: Without strict control over criteria[crit], comparing to list()
could lead to latent, nonobvious bugs later (consider if, for example,
it were a tuple instead). Suggest reversing the comparison to check
for isinstance(criteria[crit], basestring) instead.
135-148, 151-167: Very similar code blocks. Perhaps a sub-function is
in order?
174: Nit: You accept a "db" parameter, but then ignore it and use AIdb
instead. Change uses of AIdb to db. This would also make testing
easier as it allows one to pass in a mock db.
180: This function is very close to the prior one - perhaps the could
be combined, and a "clear_all" optional keyword parameter added,
which, when True, clears out existing criteria?
test_ai_database.py:
36: Will this interfere with other tests?
test_publish_manifest.py:
70: Perhaps I'm paranoid, but using a random name (os.tempnam()) would
be less likely to run into an issue with there actually being a
"/tmp/nosuchfile".
92: Looks like this should be assertEqual
143: Nit: Could/should be assertTrue(isinstance(cri_dict), dict)
EOF: Missing tests for changes to find_colliding_manifests (the
append_manifest case)
test_set_criteria.py:
246: Class should also test the case for valid options.
ai_manifest.xml:
37: Comment says two mutually exclusive groups, but three are listed.
Please clarify.
153-160: Perhaps I missed this file when I did my changes earlier, but
for consistency with our other manifest files, the packages should be
pre-pended with "pkg:/" (e.g. pkg:/SUNWcs). I also believe with bug
1637 fixed, SUNWcs & SUNWcsd are no longer required and should be removed.
179-180: Comment seems out of date.
189: Extraneous (or missing) quote.
194: Update comment.
223: Should this line be outside the comment block that ends on line
224, as it was previously?
auto_ddu_lib.c:
61: The name change here (searchall -> search_all) doesn't seem to
have a matching change in the manifest XML.
The manifest was wrong and has been changed. The correct name,
as defined in ai.dtd, is <search_all>.
auto_install.c:
395: See prior comment on package names and pkg:/ prepend.
re: pkg URIs. I will change lines 396-399 to:
package_list[0] = strdup("pkg:/SUNWcsd");
package_list[1] = strdup("pkg:/SUNWcs");
package_list[2] = strdup("pkg:/babel_install");
package_list[3] = strdup("pkg:/entire");
Is that correct?
re: SUNWcs/SUNWcsd. They should be removed when our gate merges
in the fix for 1637, right?
auto_parse.c:
156/160: Could you just add a "default:" after line 156?
Will do.
620, 1056: numberical -> numerical
Will fix.
auto-install/default.xml:
35-41: See prior comment on pkg names.
software.dtd:
55: im_type - for curiosity's sake, is there a reason this isn't
"img_type"?
auto-installer:
96-98: I don't suppose now would be a reasonable time to remove this
temporary solution?
Honestly, I'd prefer not to. Much of the AI client is due to be re-written
when AI is refactored under CUD and I think that's the time to fix
things like this. For now, I'd just like to ensure that the AI client does
the same thing it did before, but with the new schema.
Thanks,
- Dermot
text_live.xml, orchestrator-wrappers.c, orchestrator-wrappers.h:
Any particular reason these copyrights are being modified?
installadm.c:
1252-1308: Is there no way to skip processing the options here, and
instead, just pass everything directly to the called script? Same
question for the new do_set_criteria function. There's processing in
set_criteria.py to handle the option parsing, why do it twice?
test_manifest_serv.py:
69: Should there be assertions here? If this is relying on an
exception being raised, it'd be slightly preferred to enclose in a
try/except <Specific exception> clause, with a self.fail(<msg>) in the
except block. This differentiates between "expected" failures, and
completely unrelated errors.
installadm.1m.txt:
1-22: Based on other man pages (both in our gate and elsewhere),
should this even have a CDDL block?
On 08/ 5/10 03:07 PM, 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
_______________________________________________
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
