Drew and Karen:
My comments are mainly nits and related to pylint:
usr/src/cmd/distro_const/checkpoints/pre_pkg_img_mod.py:
Can you remove the unused import of Popen:
Instead of:
from solaris_install import CalledProcessError, DC_LABEL, DC_PERS_LABEL, run, \
Popen, path_matches_dtd
It should be:
from solaris_install import CalledProcessError, DC_LABEL, DC_PERS_LABEL, run, \
path_matches_dtd
On 02/21/12 14:14, Karen Tung wrote:
Hi Drew,
I looked at the changes to the DC checkpoint and the text installer
changes.
Here are my comments:
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.
- 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.
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.
- line 78: Nit: should this be initialized to None instead of 0, since
this
variable will eventually store an object?
- 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.
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.
- 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.
- line 324: update the comment for the checkpoint name?
- 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?
- 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?
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...
Thanks,
--Karen
On 02/15/12 10:30, Drew Fisher wrote:
Good morning!
Dermot and I would like to get a first-round code review for the
following PSARC/CRs:
PSARC/2012/023
<http://psarc.us.oracle.com/Archives/CaseLog/arc/PSARC/2012/023>
Interactive Installation to iSCSI
6974246 <http://monaco.us.oracle.com/detail.jsf?cr=6974246>
Automated Install should provide mechanism for setting iSCSI
initiator-id-node
7004719 <http://monaco.us.oracle.com/detail.jsf?cr=7004719>
Opensolaris LiveCD installation should give a GUI for installing
Solaris onto iSCSI LUN
7004720 <http://monaco.us.oracle.com/detail.jsf?cr=7004720>
Opensolaris text installer should give a screen for installing
solaris onto iscsi lun
7114789 <http://monaco.us.oracle.com/detail.jsf?cr=7114789>
unlabelled iSCSI drives and setting an iSCSI boot-device require
special handling on SPARC
7121245 <http://monaco.us.oracle.com/detail.jsf?cr=7121245> iscsi
paths don't translate to bootable OBP paths/strings
7132111 <http://monaco.us.oracle.com/detail.jsf?cr=7132111> sample
ai manifest file does not mention setting whole_disk in disk target
section
7132457 <http://monaco.us.oracle.com/detail.jsf?cr=7132457> Race
condition in AI involving the target discovery and the check for the
new OS Device Name
7145512 <http://monaco.us.oracle.com/detail.jsf?cr=7145512> pulling
iSCSI information from the DHCP server does not support iSCSI boot
https://cr.opensolaris.org/action/browse/caiman/drewfish/iSCSI/
Karen: Could I get you to look specifically at the text-installer
changes?
Niall/John: Could I get you both to look specifically at the GUI
changes?
Jesse: Can you look at code in $SRC/lib/install_target as that's
where all the calls to iscsiadm and libima.so exist
Outstanding issues:
- QE has not yet started a test cycle. I've been working with Angela
Li on this. I'm hoping to start a test cycle soon
- ai_manifest.4 is still being edited. I've emailed proposed changes
to Alta who will translate them into actual English
- The fix for 7132457 is slated to be tested by PIT soon for
confirmation.
ISO / USB access can be found at Hudson:
http://indiana-build.us.oracle.com/view/iSCSI/ Choose the job for
the installer you want to look at and you'll see links along the
job's page pointing to the latest ISO / USB image
Screenshots of the GUI and Text-Installer (for those not interested
in firing up an entire ISO) can be found here:
http://mox.us.oracle.com/code/screenshots/gui/
http://mox.us.oracle.com/code/screenshots/ti/
If people want to try out the ISOs, let me know and I can provide
target IP/LUN information to use.
Thanks!
-Drew and Dermot
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
