Hi Susan
Thanks for reviewing the changes. Please find the revised webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/CR7041537-2
On 11/16/11 01:04 PM, Sue Sohn wrote:
On 11/15/11 02: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
Hi Nirmal,
In general, there are a lot of small pep8/style issues that need to be
cleaned up, as we discussed offline. I've pointed out those that I
noticed. Please find the rest and clean them up as well.
I ran pep8 and fixed all the errors reported.
create_profile.py
-----------------
67 Can you update multiple profiles at once? Usage implies that you
can, but code doesn't appear to do so. If the answer is no, you will
need to modify the usage and also add a check for that in parse_options.
80, 82 indented too far over, should line up with 87
81 No space before ':', per pep8. Please go through entire file and
remove similar occurrences.
104 remove extra blank line
310 Add space after #
311 line looks longer than 80 characters
319 (old 307) See William's comment about templating code
error code exist in validate_file().
329 Need another blank line
335 indent under "Error..."
342 indented too far over
342 if you say "File does not exist: %s\n", it makes localization easier
345 if len(options.profile_name) == 0:
->
if not options.profile_name:
365 need space after #
368 indent under "Error
fixed.
374 Is there another, more direct way to get this data, other than
calling a function in list.py?
removed dependency on list.py
379 critera -> criteria, (and should that say "range criteria"?)
384 add spaces around +
386 add space after comma
387 add spaces around +
389 ditto
390 remove extra blank line
395 Should there be an error message here? If one is already provided
by validate_file, please add comment to that effect. Similar comment
for 399
comments added.
399 indentation
402 "SELECT file FROM "+
->
"SELECT file FROM " +
408 any reason you are using parens here?
414 no need for backslash, implied continuation, line up 415 under
_("Error
416 remove blank line
420 should be _() for localization
421 need another blank line
fixed.
test_create_profile.py
----------------------
35-37 alphabetize imports
38-39 alphabetize
47 ditto
48 osol_install.auto_install.common_profile is already imported on
line 42
166 extra space before =
201 need spaces around =
459 space after =
518 failUnlessEqual will be deprecated in 2.7, use assertEqual
installadm.py
-------------
238 Since you renamed get_usage in create_profile.py, you need to
update it here as well.
266 You need to add an entry to the cmds list so that update-profile
is printed as part of installadm help. You should add installadm help
and installadm help update_profile to your test cases.
As William mentioned, you should test create-profile (and help for
create-profile) for regressions.
Have you tried updating a profile and doing an install to verify that
the updated profile was used?
I have tested the cases suggested by you.
Test results :
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
Thanks
Nirmal
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
