Hi Kristina,
My apologies for the delay.
I have one general comment. I believe it would be useful to capture
at one place how particular sysid configuration scenarios map to new
sysconfig ones as well as which combinations are considered invalid
by js2ai tool. Perhaps pushing some short doc into caiman-doc
gate may help.
Having such document, it would be easier to validate if the code
accomplishes what it is supposed to do :-)
Please see my code review comments below.
Thank you very much for fixing those issues,
Jan
__init__.py
===========
1283 if not os.path.isfile(options.validate):
1284 err(_("no filename specified: %s\n") % options.validate)
1285 return EXIT_IO_ERROR
The error message on line 1284 is not correct, since
'no filename specified' case is caught by argument parser.
However, in this case the problem is that provided file to be
validated does not exist or is not a file. So I may recommend
to change error message to something like
1284 err(_("%s does not exist or is not a regular file\n") %
options.validate)
--------
1328 if not os.path.isfile(filename):
1329 err(_("no filename specified: %s\n") % filename)
1330 return EXIT_IO_ERROR
Again, the error message on line 1329 is not quite correct, as there
may be two possibilities why check on line 1328 may fail:
* filename is empty string (CR 7097822)
* filename does not exist
So base on that, I am wondering if check may be enhanced in following way:
if not options.profile:
err(_("no filename specified\n"))
return EXIT_IO_ERROR
elif not os.path.isfile(filename):
err(_("%s does not exist or is not a regular file\n") % filename)
return EXIT_IO_ERROR
conv.py
=======
197 if values[0] in ["sun4c", "sun4d", "sun4m"]:
To make code more robust, I am wondering if it may be better
to check for supported architectures instead of trying to
cover all non-supported ones (which seems more fragile) -
e.g. the check may then look like:
197 if not values[0] in ["sun4u", "sun4v"]:
conv_sysidcfg.py
================
751 netcfg = None
Is there any reason why 'netcfg' boolean variable is not initialized
to the real value - perhaps 'False' in this case ?
Then subsequent code could be simplified by removing further assignments
to 'False'.
--------
811 self.__create_net_interface(auto_netcfg=netcfg)
Since __create_net_interface() accept only one parameter, I may recommend
to simplify calls to this function by omitting explicit assignment:
self.__create_net_interface(auto_netcfg=netcfg)
->
self.__create_net_interface(netcfg)
There are more such calls withing the code (1021, 1025,...).
On 11/03/11 15:40, kristina tripp wrote:
I still need a code review for this. Sending out again as I have not
yet gotten a review. Please have all comments by COB Tues Nov 8th.
- Kristina
-------- Original Message --------
Subject: Code Review Request: multi CR's for js2ai
Date: Tue, 25 Oct 2011 13:42:01 -0600
From: kristina tripp <[email protected]>
Organization: Oracle Corporation
To: [email protected]
Good afternoon
Could I please get a code review for:
7074408 js2ai should report errors for failure on each line in rules file.
7098250 js2ai doesn't choose proper network to convert primary key is used
7097410 Teach js2ai to avoid converting obsoleted SPARC platforms found
in JumpStart rules files
7083430 nosexunit test failure for solaris_install/js2ai/conv_sysidcfg.py
7097822 js2ai fails with exception when profile or manifest path ends
with "/"
7097883 js2a incorrectly turns on Auto Network instead of Default when
name_service is set in sysidcfg
7103161 js2ai doesn't delete old sc_profile before processing new sysidcfg
https://cr.opensolaris.org/action/browse/caiman/enpointe/CR7097883/webrev/
Expanded the unit test to make sure the DefaultFix or Automatic was set
for the network based on the settings being processed.
Unit tests - PASSED
indiana-build> slim_test cmd/js2ai/modules/test
[ text -deleted]
----------------------------------------------------------------------
Ran 236 tests in 15.482s
OK
ok
----------------------------------------------------------------------
Ran 237 tests in 31.916s
OK
Additional Testing:
Performed a series of sysidcfg translation and system reconfiguration of
the resulting sc_profile using sysconfig.
As part of the root_account change for CR in addition to a sysconfig
test also diff a diff of sc_profile.xml before the changes associated
with CR and after to ensure that the only addition to the xml was the
type field
_______________________________________________
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
