Hi Sue,
On Wed, 3 Nov 2010, Sue Sohn wrote:
Hi Alok and Drew,
__init__.py
23 Copyright should be range of 2008-2010
This is actually brand new code written in 2010.
execution_checkpoint.py
28 ExecutionChkpt -> Execution ?
Changed.
131 indent Checkpoint under kwargs
Done.
169 Need docstring
These are DataObject methods just being overridden
here so I don't see the value in providing a docstring.
If you feel strongly about it, I can add it.
204 indent under "invalid
209 ditto
211 remove line
Done
pkg_img_mod.py
50 after to boot -> after the boot
87, 106 don't need "\"
112 don't need "+ \", implied continuation within parens
88 line up str under "Error
Changed.
105-106 Move up before 101, if the directory doesn't exist, the file under
it won't either
Moved.
141 line up under "Compressing
Changed.
150, 185 This seems fragile, making a decision based on the output to stderr.
You're right. I've removed this check. Now upon a failure
the user will see the following error message:
Compression of /usr file system failed.
followed by output of os.strerror(p.returncode).
118-191 create_usr_archive and create_misc_archive seem to have a fair amount
of code in common. Would it make send for them to share a common routine to
(maybe to do the lofiadm?)
I see limited value in extracting a single command into
a separate routine especially if it's being called only
a couple of times. Not doing this at this time.
176 indentation
Changed.
175, 186, 190, 244, 469 no need for "+ \", implied continuation within parens
217 PkgImgMod -> LiveCDPkgImgMod
217 after to boot -> after the boot
Changed.
370-371 Should this be moved to the top of execute? That way, if something
else
is added later that changes the dir in a call from 355-370, you'll already
have it saved.
yep.
384 - wasn't this different in the code walk-through?
Nope, but now it is. TextPkgImgMod is a separate class
since it needs to additionally create the livecd-content
file in addition to whatever AIPkgImgMod is doing.
cli.py
o A couple of commands are not alphabetized properly
Changed.
pre_pkg_img_mod.py
93,96 don't need "\", indent str under "Error
Removed.
167 I was under the impression that it wasn't necessary to set
LD_LIBRARY_PATH anymore. Do you need this?
Removed.
188 You check if the path exists, but then raise an error saying it isn't
executable?
Changed to "does not exist"
216, 286, 354, 356, 412, 469, 509 don't need "+ \"
217 line up left " with previous line
Changed.
install-distribution-constructor.mf
o Shouldn't rtc_config.default and coreadm.default be in here?
Added.
o Files should be alphabetized
Changed.
test/test_pre_pkg_img_mod.py.html
82 You need to resolve the 'XXX'
Added the check.
test_ai_publish_package.py
o add docstrings to all tests
Added.
o I prefer the self.assertTrue and self.AssertEquals constructs to the
self.assert_ construct. Seems clearer. Same for other test files.
Why are assertTrue and AssertEquals preferable?
test_boot_archive_archive.py
o add docstrings to all tests
Added.
120 should be end -> should end
Changed.
248 -249 Looks like this isn't quite finished yet?
Removed the XXX.
dc_livecd.xml (and other xml files)
99 Would be good to have a comment explaining the notation.
I don't think it's worth explaining the notation here
in the manifest since the users will almost never be
changing this path. The docs however will talk about it.
321 Don't forget to take care of this (and other files too)
Yep.
Thanks again for your review, Sue.
Alok
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
