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
>
Hi Keith,
Thanks for taking the time for the detailed review. Replying to
test_ai_database.py, test_publish_manifest.py, test_set_criteria.py,
installadm.c, and installadm.1m.txt.
> test_ai_database.py:
>
> 36: Will this interfere with other tests?
We discussed this off line and agreed not to change this.
> 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".
Will change here and in test_set_criteria, but since os.tempnam() printed out a
security warning, I will use tempfile.mktemp() instead.
> 92: Looks like this should be assertEqual
Yes, thanks for noticing that.
> 143: Nit: Could/should be assertTrue(isinstance(cri_dict), dict)
Or maybe even assertTrue(isinstance(cri_dict, dict)).
> EOF: Missing tests for changes to find_colliding_manifests (the
> append_manifest case)
I'll look into writing something for this.
> test_set_criteria.py:
>
> 246: Class should also test the case for valid options.
Will add this.
> 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?
Good point. Will change for both.
> installadm.1m.txt:
>
> 1-22: Based on other man pages (both in our gate and elsewhere), should
> this even have a CDDL block?
Will remove to be consistent with other man pages.
Thanks,
Sue
> 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
