Hi Ethan,
Responses inline.
On 08/10/10 07:51 AM, Ethan Quach wrote:
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.
That's fine. The only potential issue is that it's more difficult to
change the 'type' of an accepted parameter later, as you have to start
updating the callers.
[...]
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.
If that's possible to update, that'd help debugging if it ever came up
(though I imagine it's highly unlikely in any case).
[...]
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.
None of the criteria *now* are case-sensitive filenames - but can you
guarantee that none of the values in the future will ever be case
sensitive (files or otherwise)?
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.
Seems I misread the code initially - disregard that.
[...]
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.
Thanks! Forward progress is good.
[...]
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.
Ok.
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().
Ok.
760: Nit: Just say "__getitem__ = get_criterion"
OK. Does it need to be self.get_criterion, or just get_criterion?
Just get_criterion (defining it at the class level, not the instance level)
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.
Thanks.
[...]
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"
Sounds good.
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"
Sounds good.
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?
Sounds 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.
Modules can be passed around as parameters, so this is still doable if
you'd like (though not needed). The unused db parameter should be
removed, though.
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.
Sounds good.
Thanks,
Keith
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
