Hi Keith,
Please see my responses to a few items inline.
I removed all items that I have no further comments on.
Thanks,
--Karen
On 01/ 3/11 11:10 AM, Keith Mitchell wrote:
Hi Karen,
Responding to the other items in-line.
- Keith
On 12/30/10 12:17 AM, Karen Tung wrote:
- I think it would be useful to check and make sure the
value returned from doc.get_descendants is not None instead of just
assuming
that it is valid.
In most cases, if this value comes back as non-None, there's really no
recovery possible. Originally, this threw an ObjectNotFoundError -
these lines need to be updated now to set the "not_found_is_err=True"
keyword on calling, now that it changed.
OK.
- Would it be useful to define a helper function for getting the
install profile obj so we don't have the exact same code all over
the place?
I understand that it is just 2 lines, but it would still be useful
to just make 1 change instead of 10+ changes, if something needs to
change.
Seems reasonable. I actually did just that for the ConfigProfile; no
reason the same couldn't be done here. Although I'd actually expect
the InstallProfile to disappear when the CUD/DOC 'target' objects are
used in text installer. so the long term value might be limited.
In the long term, I envision the CUD/DOC target objects will be used for
instantiating the target.
However, the values currently stored in the install profile is still
needed. For example, to
display the values in the Text Installer summary screen, and possibility
needed by the actual
installer application when it needs to set the timezone...etc..
Specific comments:
---------------------------
> usr/src/cmd/text-install/osol_install/text_install/__init__.py
- line 35: why are you doing "del gettext" here? It would be useful
to put a comment here explaining the reason.
We can add a comment; though the line should probably just get
removed. It's not terribly valuable in the grand scheme of things.
If that's the case, I supposed it is best to remove it. I was confused
about what it is doing.
So, I asked. :-)
> usr/src/cmd/text-install/osol_install/text_install/summary.py
- lines 55-56: same comment regarding hard coding the full help file
path.
- lines 151-152: I think it would be better to define somewhere,
perhaps
somewhere in the sysconfig module,
constants for indexes in the array for the root and primary user,
so, we don't
need to remember that 0 means root and 1 means primary user.
I agree that the current method isn't best. I'll consider options for
making this better. I'm not certain hard-coding 0 and 1 in a different
spot is much better, to be honest - at one point we discussed using a
dictionary of username to UserInfo object to store users, but that's
difficult to use because (a) the primary user's login isn't easy to
determine, and (b), with DOC, that sort of query isn't possible (DOC
objects have a "name" attribute, but that's set at object creation
time; so can't be changed if the person entering the data changes the
username).
At install time, we are only concerned about a maximum of 2 users, root
and the primary user.
So, using 0 and 1 or whatever other hard coded scheme should be OK. I
am not sure
whether the user related objects you defined for the sys config project
will be used for purposes
other than installation. If so, the hard coding of indexes probably is
not a good idea.
> usr/src/cmd/text-install/osol_install/text_install/text_install.py
- line 149: why is the loglevel hard coded to 0, instead of the
value in options.log_level?
I can't recall why I did that. It was either:
(a) A conflict with other logging services that will get resolved when
the text installer is fully CUD'ified to use just one approach to
logging, or
(b) a typo / left-over from testing.
- lines 155-156: "no_install_mode" and "is_x86" are not defined in the
install_profile object. Should they be. As with the previous comment,
it would be useful to define them and to print them out in __str__
for debugging.
Yes, when the text installer was written, I should've had those in the
__init__ methods at minimum, and possibly in the __str__ method.
However, I'd expect "no_install_mode" to get deprecated for the
engine's "dry_run" when text installer is updated to CUD. "is_x86"
appears to only be used in by the reboot function in this same file;
so it can simply be removed entirely, I think.
Sounds good. If you can remove the is_x86 flag right now. I will
determine what to do with the no_install_mode when we fully change the
text installer over to using CUD.
- line 160: I know there's no harm to register the checkpoint here.
But would it make the code easier to read to register it before you
need
to run it?
My understanding is that checkpoints should, if at all possible, be
registered all at once. Since Target Discovery would need to be run
right away, the registration should occur here, when the program
starts up.
In general, that's the case. When we fully change the text installer
over, I will most likely
put the registration here. For this particular putback, since you are
just registering
and running 1 checkpoint, I thought it will be easier to the person who
will later be
updating the code to have all the stuff that need to be re-shuffled in 1
place.
It is totally up to you guys, not very important to me.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
