[Resend due to delivery failure]
On 01/27/11 11:21 AM, Keith Mitchell wrote:
On 01/26/11 11:25 AM, Karen Tung wrote:
Hi Keith,
Please see my comments inline.
On 01/26/11 10:30 AM, Keith Mitchell wrote:
On 01/26/11 09:40 AM, Karen Tung wrote:
Hi Jan and Keith,
I checked all the files I previously have comments on, and my
comments are
addressed. I noticed the following 2 minor items.
usr/src/cmd/text-install/osol_install/text_install/__init__.py:
- lines 34-40: Is there a reason to put all these lines here
instead of
after all the "import" and "from" lines? Normally, assignment lines
appear after all the import stuff.
The modules imported below need to be able to reference those
assignments. The alternative would be to move those assignments to a
different module, but I'm really not sure where else they would go.
Since they have to be there, it is OK to leave them where they. I
was just confusing to me
to see that they are kinda out-of-place. It would be useful to put a
comment there
specifying the importance of the order so people in the future won't
try to move them
to what they think is the "right" place.
Added. (Though note that anyone who attempts to move it would find out
*very* quickly why it is where it is!)
-line 281: This call for getting the install profile doesn't have the
second argument, "not_found_is_err=True". Why?
The InstallProfile is only retrieved there for the purposes of
dumping debugging data to the log. Its absence is not necessarily
fatal at that point.
It's true that it's absence won't affect the debugging statement
below for printing it out.
However, it's absence indicate that something is wrong, since the
install profile is
inserted into the DOC by the prepare_engine() function, so, there's
no reason why it
would be missing.
Perhaps, perhaps not. It's rather trivial to change, so I'll go ahead
and add the 'not_found_is_err' flag.
Thanks,
--Karen
Thanks,
--Karen
On 01/24/11 12:23 AM, Jan Damborsky wrote:
Hi,
thank you all who provided us with valuable feedback
for the first version of System Configuration Interactive tool.
The updated webrev addressing all comments received has been
published:
http://cr.opensolaris.org/~kemitche/sci.1.2/
We also wanted to publish incremental webrev and Keith spent
significant amount of time
trying to generate that. However, due to the merges with different
gates (CUD, slim_source)
we didn't find out how to create 'clean' incremental webrev. But
we are at least providing
following one:
http://cr.opensolaris.org/~kemitche/sci.1.2.diff
This appears to be correct, with the caveat that, due to the above
issues, it "looks"
like we're re-adding old DC, and it also picks up merges from
slim_source. The diffs
in the SCI-touched files are, however, accurate. Where not sure,
please use full webrev as the referential one.
The updated webrev in addition contains SCI unit tests and fixes
for following two bugs:
7004649 InstallLogger doesn't propagate log levels appropriately
7002621 The initial user configuration needs other entries
We would like to ask you if you could please take a look at updated
webrev and let us know if we neglected to address some comments
or if you feel anything else was missed.
Thank you very much,
Jan
_______________________________________________
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
