Thanks for taking the time to review this Jan. Comments follow
On 11/4/2011 3:31 AM, Jan Damborsky wrote:
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 :-)
Agreed. There was functional doc that Evan and I did initially to
outline what values we would convert in js2ai and what those conversions
looked liked but I gave upon trying maintain. Instead I opted to
output the xml code that we would output in a code block in conversion
routine being executed. In a perfect world I would like to spend the
cycles and to have a general design doc but since I'm technically
suppose to be working on another project now I don't see this happening
anytime soon. But one never knows, js2ai just keeps pulling me back
for more tweaks as I keep learning about little things that need to be
addressed in the conversion process that have gone undiscovered due to
lack of hardware resources and testing resources.
What keywords we support for conversions however is documented in the
js2ai man page.
By scenarios I assume you mean something along the lines of when
name_service=x and net_interface=y we do z. For sysidcfg conversions
hat was never in the plan because originally we never envisioned that
there was an interdependency between tags in sysidcfg. This has only
recently changed when you or William pointed out that if a name service
was configured the network could not be Automatic and DefaultFixed was
required. This information will eventually need to get documented in
the js2ai man page.
For profiles there is a interelationship between some of the tags used
to convert. This information is captured both in comments in the code
and in the js2ai man page. In the js2ai man page see "How the System's
Root Disk is Determined During Profile Translation" and "How the any
Device Is Translated During Profile Translation"
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)
Thanks. That's a good suggestion. Fixed
--------
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
Good point. Code changed
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"]:
I'm going to leave this alone as the unsupported architectures are a
known entity at this point in time. What will be supported in the
future is an unknown. As I understand it not all sun4u platforms are
supported in Solaris 11
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'.
Not that I can see. Your suggestion is a good one and I've implemented
the required changes
--------
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,...).
Done
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
