Hi Drew,
Please see my response inline.
On 02/21/12 03:03 PM, Drew Fisher wrote:
On 2/21/12 3:14 PM, Karen Tung wrote:
Hi Drew,
I looked at the changes to the DC checkpoint and the text installer
changes.
Here are my comments:
o
usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py:
- line 176: I think this is too late to check whether
/etc/default/dhcpagent
file exists or not at this point? The whole function depends on
the existence of the file. I think we should check the first thing
we enter this function, and exist with error immediately if
the file doesn't exist.
/etc/default/dhcpagent is delivered by pkg:/system/network, which, I
think is part of the "core" package list and always installed. If you
feel strongly about it, I can put a check in the method.
Actually, I do not have any preference on having the check. I bring up
the point
because you are doing the check on line 176. Since that file has to be
there, why don't
you remove that check. If the file doesn't exist for some reason, at
least the
shutil.copy2() call will fail.
- line 171-172, I think it's better to dynamically
figure out the directory of the file you want to save
and create that instead of hard coding it here.
None of the other "save" methods dynamically figure out the basepath
to the file(s) they copy over. Maybe something like this (dynamically
figure out the basepath), can be done with CR 7093060 (on my todo list
for after these projects integrate). Would you be ok with deferring
this issue until 7093060?
Yep, no problem.
usr/src/cmd/text-install/discovery_selection.py:
- line 76: is it necessary for "self.choice" to be a variable for the
whole class? This variable is only used in the "on_change_screen()"
function.
Fixed to no longer be an instance variable.
- line 78: Nit: should this be initialized to None instead of 0,
since this
variable will eventually store an object?
Fixed.
- line 92, line 101, line 117: Make the item_key strings constants for
the DiscoverySelection class? I think that will make them easier
to maintain and use.
I don't see a need to turn simple strings like "iSCSI" and "local"
into globals. There's simply no need to do that here. It forces
Python to make unnecessary lookups and it makes your KSH show. ;)
IMO, it's better for them to be string constants so it's more
maintainable for the future.
For example, if you change the item_key string from "iscsi" to "iSCSI"
for some reason.
Now, you need to make sure to find everywhere else that tries to match
that item key.
If you miss one spot, you will get a bug. If they were string
constants, you just change
the one constant, and everything else will not need to be changed. I
think defining them
as constants will be less mistake prone for the future.
usr/src/cmd/text-install/disk_selection.py:
- line 246-247: can you put a comment here explaining why we can
immediately
return if there's not an iscsi object in the DOC? It took me quiet a
while to
realize that we always create an iscsi object if the user chooses iscsi
in the previous screen.
Sure thing.
- line 265: can you put a little more detail on why the kwargs needs
to be
updated for the case where the iscsi target discovery checkpoint
already exists.
Sure thing. For those curious, target discovery has always had the
ability to discover a single disk via a ctd string. Since the engine
does not allow us register multiple checkpoints with the same name, we
can simply update the checkpoint's kwargs with the new ctd string and
re-run the checkpoint. This functionality lets the user select one
iSCSI LUN and then if needed, go back and select a second LUN (or a
third, etc.) while using the same checkpoint over and over again.
- line 324: update the comment for the checkpoint name?
Fixed.
- line 523-549, question: if the user choose to go back screens after
he/she
has completed target selection, which means, there might be iscsi
Disk objects
in the desired target tree, when will those be removed?
They're removed by line 538:
536 for disk in
discovered_target.get_descendants(class_type=Disk):
537 if disk.ctd == self.iscsi.ctd:
538 disk.delete()
Well, that line removes the "disk" from the *discovered* branch.
What about the *desired* target branch. Does it need to be deleted from
there?
Or is it that target controller "magically" handles that condition?
Thanks,
--Karen
- line 549: I think it's kinda odd to be modifying the target
controller's private
variable here. Should we implement a better way to "reset" the
target controller?
Probably. We should also implement a better target controller while
we're at it. :) Seriously answer: I'm not sure why that attribute
is private. There's nothing in it that requires it to be so. The
reason I'm accessing the private variable is there's a @property to
return that list but no setter for it. This causes it to be read-only.
usr/src/cmd/text-install/summary.py:
- lines 235-240: These looks like instructions on what the user
should do after reboot.
However, this is just a summary screen, I don't think it's intended
for post reboot
instructions. People mostly would forget what they saw on that screen
when the install is done. I think it would be more useful to
add these instructions to the screen where we inform the user that
the install is successful...
Ooh. That's a much better idea than what I had. I'll make that change.
Thanks for the review, Karen!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
