Nirmal,
Please find my responses inline:
On 11/18/2011 11:48 PM, Nirmal Agarwal wrote:
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).
In fact, you eliminated redundant code.
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.
However unlikely it may be, another user could delete the profile
database record after the initial check and before the profile record is
read again with this getResponse (now at 418), so the check for None is
still desirable.
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.
429 - direct 'print' to sys.syserr
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.
Does it still update if the profile name, -p, is omitted, defaulting to
the basename of the filename, -f? It appears that you have indeed coded
it on 347-350.
nit:403 - align the parameter on this continuation line to fall directly
underneath the first parameter after the parenthesis on the previous
line; i.e., 'verbose' should fall directly under 'profile_name'
William
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
