On 01/ 3/11 04:54 PM, Karen Tung wrote:
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..
Configuration is all stored in the ConfigProfile now; the only items
left in the InstallProfile is target related; the summary screens could
easily query against the target objects directly - I don't see a need
for the "InstallProfile" once the new target objects are used.
(We can talk more tomorrow as well; I may be misunderstanding your point)
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.
Yeah, I suppose taking your suggestion would be good enough for now.
This is something that can evolve later if needed.
> 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.
Will do.
- 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.
I'll leave it up to Jan. Whoever follows in our wake to move text
install to CUD will need to deal with the prepare_engine() function
either way, though.
Thanks,
Keith
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
