Hi William
Thanks for the review.
Please find the updated webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/CR7041537-4/webrev/
diff webev:
http://jurassic.us.oracle.com/net/indiana-build/export/home/na210770/ai/7041537/slim_source/webrev.diff
I have tested the same and works as expected.
Thanks
Nirmal
On 11/22/11 04:17 AM, William Schumann wrote:
Nirmal,
Your changes look fine, except for one detail that is best practice
for the localization engineers that should be in the original code.
When localized strings are translated, the most natural ordering of
the parameters may be different in the target language. Keyword
arguments in the Python format string syntax can be used so that
localization can change the order only by changing the relative
position of the keyword arguments in the format string.
http://docs.python.org/library/string.html#format-string-syntax
So, for the error message that has two arguments, it is better for
localization to do this:
raise SystemExit(_("Error:\tService {service} has no profile named
{profile}.").format(service=options.service_name, profile=profile_name))
or break it up to make the code more readable:
missing_profile_format = _("Error:\tService {service} has no profile
named {profile}.")
raise
SystemExit(missing_profile_format.format(service=options.service_name,
profile=profile_name))
So localization is free to reverse the order of 'service' and
'profile' only by changing the format string.
Suggest changing both occurrences of these and retest with a
non-existent profile name.
Otherwise, your changes are fine.
William
On 11/21/2011 9:07 PM, Nirmal Agarwal wrote:
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
