Hi Jack -
Thanks for your review. I've responded below.
Ginnie
On 02/ 9/11 01:19 PM, Jack Schwartz wrote:
Hi Ginnie.
Here's group 1. The code looks pretty clean and clear. Nice job.
src/Makefile.master:
--------------------
OK, but...
- Question to the wider group: any reason why the
ROOTPYTHONVENDORINSTALL... and ROOTPYTHONVENDORSOLINSTALL... items are
not grouped together and in alphabetical order?
src/Targetdirs:
---------------
Please put the ict line behind the js2ai line. The manifest line is
also out of order. Can you position it please even though you didn't
originally place it?
lib/Makefile: OK
-------------
lib/Makefile.targ: OK.
------------------
Not your problem, but the same comment about the ...SOLINSTALLMANIFEST
statements being out of alphabetical order.
I'll fix all of the above.
lib/install_ict/Makefile:
-------------------------
SUBDIRS is not specified. To build the install_ict/test subdirectory,
specify it in the Makefile as:
SUBDIRS= test
As is, the $(SUBDIRS) line has nothing to do.
There isn't any reason to build the test director, so would it probably
make more sense to remove SUBDIRS.
lib/install_ict/__init__.py: Please consider putting the contents of
common.py into this file. My understanding of __init__.py is that it
is a good place for keeping package-wide (i.e. common) utility
definitions or declarations.
Ok. I'll give it a try.
lib/install_ict/apply_sysconfig.py:
-----------------------------------
Nit: indentation on 97 and 99.
Will fix.
Nit: 110, 123: instead of hardwiring 3 for sys, use
grp.getgrnam("sys") to get it instead. I'd say the same for root too,
but everyone knows root is zero :)
I'll change that.
119: Will the new install-gate Popen work here?
See /usr/src/lib/install_common/__init__.py
Yes...I made a note to go back and change the subprocess stuff, and then
forgot to follow up.
Handling the error coming back from the subprocess call would help the
user more easily decipher any errors. For example, by default, the
following prints if the default CalledProcessError exception is raised
for "ls <garbage-file>"
>>> subprocess.check_call(["/usr/bin/ls", "asdfasdf"])
asdfasdf: No such file or directory
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python2.6/subprocess.py", line 488, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/ls', 'asdfasdf']'
returned
non-zero exit status 2
That's a lot for the user to wade through...
I have to revisit these when I add in the install-gate Popen, so I'll
take this into consideration when I do.
Nit: Would it fail more cleanly if the file copy/chown/chmod was done
first before the SVCCFG? This way SMF isn't set up for a missing
file, in case the copy bombs.
I see what you are saying. I followed the order that this was executed
in the original ICT, so I don't know if there is a reason for the
particular sequence. If you don't have any strong objections, and for
the sake of time, rather than spending time exploring it, I would rather
leave it in its current order..
Just curious: line 74 says if dry_run is true, "the log message
describes the checkpoint tasks." How much detail, and where is this
implemented?
The log message for the checkpoint is at line 80.
lib/install_ict/test/test_apply_sys_config.py:
----------------------------------------------
Nit: 41-42: can these be combined onto one line and still fit?
I'll double check and move it if I can.
92: What purpose does this line serve?
94: Would it be more helpful to call assertTrue(False) with the
message/info from the received exception, or perhaps just let the
raised exception percolate up?
If the execute command succeeds, then the test will succeed, if it
fails, then line 94 will cause the test to fail. assertTrue verifies
that the function call returned successfully or not. I not really
testing whether or not a particular exception is raised.
lib/install_ict/boot_archive.py:
--------------------------------
Same question asfor apply_sysconfig.py about dry_run tasks being
logged. Where is this implemented?
Line 75.
94: Same issue as apply_sysconfig.py about handling
subprocess.check_call() errors.
Yes.... I'll take this into consideration when I add the new Popen
104: Does update_archive really take only a second to run? I thought
it took longer. You may want to time it and give a more accurate
approximation. (How's that for an oxymoron? :) )
I think I'll add a comment that these need to be more accurately
estimated, because I don't really know how long they take.
lib/install_ict/test_boot_archive.py:
-------------------------------------
Nit: 41-42: can these be combined onto one line and still fit?
I'll check.
I see setUp() and tearDown() here is almost the same as in
test_apply_sys_config.py. Does it make sense to stick the common code
in a superclass which both of these classes (and perhaps other test
classes which may do the same setup as well -- I haven't looked at the
others yet) would subclass?
I'll give it a try and see how it works. There is a lot of commonality
in all of the tests.
92, 94: Same comments as test_apply_sys_config.py about calls to
assertTrue.
See above
lib/install_ict/common.py:
--------------------------
Content looks OK, but may be cleaner to add to __init__.py instead.
I'll give it a try.
src/pkg/manifests/system-library-install.mf: OK
--------------------------------------------
Thanks,
Jack
On 02/ 8/11 11:38 AM, Ginnie Wray wrote:
Hi -
I've need a code review for the ICT conversions. They are relatively
short, so I thought I would group them and ask a few people each take
a group.
Because this is short, I would appreciate feedback by February 16th.
If that doesn't work, let me know and I can accomodate you.
Webrev is located at:
http://cr.opensolaris.org/~ginnie/icts/
Thanks,
ginnie
Group 1:
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_ict/Makefile
usr/src/lib/install_ict/__init__.py
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_ict/common.py
usr/src/lib/install_ict/apply_sysconfig.py
usr/src/lib/install_ict/test/test_apply_sys_config.py
usr/src/lib/install_ict/boot_archive.py
usr/src/lib/install_ict/test/test_boot_archive.py
Group 2:
usr/src/lib/install_ict/cleanup_live_cd.py
usr/src/lib/install_ict/test/test_cleanup_live_cd.py
usr/src/lib/install_ict/create_snapshot.py
usr/src/lib/install_ict/test/test_create_snapshot.py
usr/src/lib/install_ict/update_dumpadm.py
usr/src/lib/install_ict/test/test_updatedumpadm.py
Group 3:
usr/src/lib/install_ict/device_config.py
usr/src/lib/install_ict/test/test_device_config.py
usr/src/lib/install_ict/generate_sc_profile.py
usr/src/lib/install_ict/test/test_gen_sc_profile.py
usr/src/lib/install_ict/initialize_smf.py
usr/src/lib/install_ict/test/test_init_smf.py
_______________________________________________
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
