Hi Alok and Drew,
Here are my comments from reviewing the updated code:
---------------------------------
boot_archive_archive.py:
---------------------------------
251-253, 258-260, 263-265, 419-420: You are hard coding the transfer
actions and types strings
here. Those are now defined as constants in
solaris_install.transfer.info, please use the constants.
---------------------------------
boot_archive_configure.py:
---------------------------------
420-430: I think it is better to use the "UserattrFile" class from
pkg.cfgfiles than to manipulate
the /etc/user_attr file yourself.
412: nit. The name of this function is called "configure_user_attr",
but this function does more
than configuring user_attr. It also configures the sudoers file. I
think a more general name
should be used.
---------------------------------
pkg_mg_mod.py:
---------------------------------
185-191, 222-227: Is there a reason the stderr is being ignored. It
would be useful to log stderr into the
log file too, if any.
264-267: I think it would be more efficient to do the equivalent of this
in Python than doing it
with calling the find command. In fact, this used to be in Python, when
I ported the code
to a finalizer script, I changed it to a command since I was
implementing a ksh script.
---------------------------------
pre_pkg_img_mod.py:
---------------------------------
348: is it necessary to generate the image size for the AI image? AI
doesn't use
the cpio based transfers, so, I think it is not needed.
---------------------------------
dc_livecd.xml:
---------------------------------
line 78: should there be a comment here with a warning telling ppl to
not add anything
after the "with screen reader" grub entry, like the current Live CD
manifest?
Thanks,
--Karen
On 11/29/10 14:16, Alok Aggarwal wrote:
Just a quick reminder to everyone intending to do
a second pass through, the deadline for providing
review comments is COB, December 1st.
If you intend on reviewing and need more time, please
let me know as well.
Alok
On Thu, 18 Nov 2010, Alok Aggarwal wrote:
Thanks to everyone who reviewed the code for the
DC->CUD project the first time around. Here's round two
that incorporates most of the changes brought up.
Incremental webrev against the initial webrev:
http://cr.opensolaris.org/~aalok/cud_dc-diff2/
Complete webrev:
http://cr.opensolaris.org/~aalok/cud_dc-2/
Some of the most significant changes included here
are:
- DC Logging has been overhauled. logger's transfer_log()
was enhanced as part of CR 7000990 and DC now uses that
instead of managing log files, etc by itself.
- Determining the ZFS mountpoints: This code was reworked
to utilize the zfs code in install_target. The mountpoints
are now determined *after* TI has executed instead of trying
to determine mountpoints ahead of time.
- DC Lockfile: This is now a context manager (used via the
"with" statement) and has added additional streamlining
- Unittests: There are now unittests for the primary DC
application. Coverage is ~ 70%
- A SimpleSchemaElement class has been defined for use by
.. well, simple schema elements. This code will eventually
be moved to the DOC code (there's an outstanding bug on this).
- kwargs/args in execution_checkpoint.py is now a separate class
- Sort files are now included in the source
- Manifest Parser tests that were commented out due to the absence
of DC in slim_source have been uncommented.
Please take a look and provide your feedback by COB, December 1.
Thanks,
Drew and Alok
_______________________________________________
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
