Keith,
Thanks for the comments. Replying to:
AI_database.py
publish_manifest.py
set_criteria.py
On 08/09/10 12: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?
Sure I could make that change, but I don't see that huge
of a benefit of doing that now than changing it to be so
if/when we'd ever need it be a list.
334: Nit: Use +=
OK.
publish_manifest.py:
62-74: Nit: The indentation on line 65 is inconsistent with lines 68,
71, and 74
Will fix that.
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.
OK, will make that change.
92-93, 97-98: Just use "parser.error()" - it automatically prints the
usage and then exits.
OK.
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...
yeah, I'll make that change.
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).
If the SMF property group lacks either of those properties,
there isn't an exact prescription for a fix. I don't think
deleting the service would the the best thing to tell users
to do either. We could perhaps include the name of the
property that failed to exist to be a little bit more informative.
138-142: PEP-8 violations: No spaces around "=" in keyword arguments.
831-833: Ditto.
I'll fix these.
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)")
I'll add a note in the docstring.
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.
No, values for criteria aren't filenames. Perhaps you're thinking
about the -C option, which is a filename.
160: Unnecessary check - OptionParser will ensure that options that
require arguments have them.
But if the argument is given as, "-c =foo" OptionParser doesn't
catch that. The check here gives a little more distinct error
message in what's wrong.
171: Nit: More informative message of the form "Criteria must be of
the form <criteria>=<value>" would be preferred.
I can change that.
302-304: Inconsistent indent (tabs?)
No, they are all spaces. It seems like 303 just needs 4 more spaces.
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...)
I like it without the "+" as well. In most other places, I don't use
it, but there are some places like this one that just didn't get
updated to remove it. Thanks for pointing it out, I'll change it.
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.
See reply to your related comment below.
Nit: Need a space between % and err at the end. Also happens on line 646.
Will fix both of these.
631, 696: "open()" is preferred to "file()"
Will change.
634-637: Unneeded. Let 643-644 handle it.
Will remove.
655, 666: Again, something other than SystemExit please - particularly
for a new function where there's no existing usage.
This actually isn't a new function. I've just made modifications
and moved it from line 1025 of the old file. And believe it or not,
its only usage previously was rooted from options parsing.
I do agree with you in principle though, and since I did move it
out to a function, I'll make the change you request for this
method and the new verifyCriteriaDict() as well.
Assuming criteria_dict is from the criteria_to_dict() above:
684: This comment is incorrect
Will fix.
705: This could be "for name, value in criteria_dict.iteritems():"
Will change.
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.
The intention in criteria_to_dict was to do just enough
validation to get things into a dictionary, i.e. the input
was of the form <criteria>=<value>.
I think you've caught the only lines here in verifyCriteriaDict()
that does somewhat of a redundant check with criteria_to_dict(),
but the function is separately callable, so wanted to make it
self reliant in that respect at least. This is why it raises
an AssertionError there.
723: Statement would be more obvious if it were "if len(range_value)
== 1:"
OK, can make that change.
749: Given the functions defined here, it seems like it'd be more
appropriate to subclass dict.
I'd much rather not touch this class at this point; it was
existing and I didn't change anything in it other than
renaming it from _criteria() to Criteria().
760: Nit: Just say "__getitem__ = get_criterion"
OK. Does it need to be self.get_criterion, or just 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.
OK
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...
I know I added a few of these; I will look at doing the
latter approach.
1450-1451: Nit: "self._criteriaroot.getroot().extend(ai_criteria)"
should work
Will change.
set_criteria.py:
54: Need a space after the colon in "append:" for this to show up
right when printed.
58: ditto
Will fix
72: See prior nit on not needing this if/else clause.
OK.
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?
What? The OptionParser isn't smart enough to do that
for us already? :-) Seriously though, that's probably a
good idea. I'll do that here and in publish_manifest as
well.
80, 87: Use parser.error()
done.
92: Would prefer to see this function return True/False, and have the
caller exit appropriately. (See above on SystemExit)
Agreed, I can make this change.
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?
Actually, this could be very common; it happens when the
user mistypes the name of the manifest. Given the previous
comment, I think we should just return false here, and the
caller will output the message "Error: install service does not
contain the specified manifest: %s"
114: I'd print the full path queried so the user knows exactly where
to look if needed.
Sure, we can do that.
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.
No problem. I'll change "nvpair" to "nvstr"
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.
Sure, I can change that here, though there are other places
where this is also being done like line 198 (and a couple of
other places in publish_manifest.py). For those cases, there
is a blind else which assumes that criteria[crit] must be None,
so making the else assume an iterable doesn't work for those
places. We could change those places to check for None
explicitly, elif isinstance(basestring), else fall to the
assumption that its an iterable. That sound reasonable?
135-148, 151-167: Very similar code blocks. Perhaps a sub-function is
in order?
Yes, I think that is doable.
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.
The name is perhaps misleading, but db is not an instance of
an AIdb object. AIdb is the imported AI_database module.
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?
(I meant to come back to merge these at some point )
In the else at line 240, if we delineate between the two modes
and simply not fill in NULLS for "append" mode, and do fill in
NULLS for "set" mode, we get what we're looking for. I'll
make this change.
thanks,
-ethan
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
