Hi William
Thanks for the review. Please find the updated webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/CR7041537-2
On 11/16/11 11:38 AM, William Schumann wrote:
Nirmal,
The main code path of create_profile.py has been modified. Manual
testing of 'installadm create-profile' with some variations is
necessary, if this was not done.
create_profile.c:
old 307-326 - the code to inform the user of misuse of template
variables has been removed. Suggest restoring the code that was
removed and exercise by providing using invalid template variable
names and by using template variables that are valid, but not defined
in either the user's environment or in criteria.
This functionality exist in validate_file() in DataFiles.py I have
tested the same (manual tests).
332-335 - Although you will see this check for root in older code, the
preferred method of handling this is reporting failed exceptions - the
reason being that the install code should not hard-code security
restrictions that may change. For example, the shutil.copyfile
exception handler at line 414 will catch general IO errors. Suggest
checking the exception reason and printing a different message if an
access violation occurs. See how this is done elsewhere in the module.
I have removed the check for root.I believe I have taken care of
reporting errors in the code. Let me know if I missed something.
Also, I will be fixing the permission to execute the command with the
fix for CR 7108281.
374 Avoid creating unnecessary interdependencies like this one, which
makes update_profile.py dependent on list.py. Code that becomes
common to multiple modules should be moved to common_profile.py. But
rather than doing that, try using getCriteria() from AI_database.py
(imported here as AIdb).
Thanks for pointing it out. Fixed.
408 - The return of getResponse() should be first checked for None.
This can occur with an unexpected database failure and referring to an
element will then generate an unhandled exception.
I already checked the existence of profile file in the db at line# 368
which confirm the presence of element in db. Let me know if you still
feel the need of check.
420 Error messages should be localized. Just put the message as the
only argument to function _(), which uses the gettext localization
module and signals the translators (humans, that is) to translate the
string. Direct user message output to sys.stderr. See other user
messages in this module.
Fixed.
Test results location :
Manual-tests ---
/net/indiana-build/export/home/na210770/ai/7041537/slim_source/manual_test.result
slim-test ---
/net/indiana-build/export/home/na210770/ai/7041537/slim_source/slim_test.result
Let me know if I missed some tests.
Thanks
Nirmal
Thanks for improving the tests in create_profile.py.
William
On 11/15/2011 11:53 PM, Nirmal Agarwal wrote:
Hi all
Could I please get a code review for the following CR :
7041537 It will be nice to have an installadm update-profile command
Webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/CR7041537
Test Results :
slim test
---------
results stored at :
/export/home/na210770/ai/7041537/slim_source/test.result
manual tests :
-------------------
ran "installadm update-profile " with "-f" option and profile without
templates and with templates.
Error out if the new profile contains templates not present in the
criteria of the profile.
Let me know if I need to run some other tests.
Thanks
Nirmal
_______________________________________________
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
