Hi William
Please find my responses inline:
On 11/21/11 05:46 AM, William Schumann wrote:
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.
I have fixed all the comments provided by you. Please find the new webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/CR7041537-3
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.
Yes It will take the basename of the file as profile filename and will
fail if cannot find profile in the database. If it finds one it will
update the profile. I have tested the same.
Thanks
Nirmal
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
