Hi Keith,
On Tue, 23 Nov 2010, Keith Mitchell wrote:
Hi Alok,
Everything is looking really good to my eye. I have just a few last minute
nits and minor questions.
Thanks for taking another look.
General nit: Some of the added logging statements are uncapitalized. I don't
know if that bothers anyone besides me though.
distro_const/__init__.py:
135: keyword argument defaults should be static, due to a quirk in how they
work. So, baseargs should default to "None", and then, in the first line of
the function, be overridden to "sys.argv[1:]" if it's been set to None.
(Probably not a big deal for this particular function, since it likely won't
be run twice in the same process, and the variable is only read, not returned
or modified)
Changed.
grub_setup.py:
381: Nit: Add a note in the docstring that this is a text install specific
function?
Added.
pkg_img_mod.py
159-163: Can you confirm that this is still actually needed and desired, per
bug 14559?
Yep, this is indeed needed. The system doesn't boot if
this symlink is absent.
pre_pkg_img_mod.py:
217: Does LD_LIBRARY_PATH need to be unset here, or is the comment out of
date?
Comment is out of date, removed.
install_utils.py:
set_http_proxy: If the http_proxy environment variable is already set (e.g.,
the user ran something like: "HTTP_PROXY=http://example.com distro_const
build manifest.xml"), should that take precedence?
I think there are two cases here:
a) A user has HTTP_PROXY set in his environment as well as
the manifest and wants the former to be honored.
b) Same as (a) but the user wants the latter to be honored.
There doesn't seem to be a way to discern which of the two
the user desires. I think it's simplest to always allow
what's set in the manifest to take precedence.
Thanks,
Alok
Thanks,
Keith
On 11/18/10 02:27 PM, 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
