Hi Chris and Andrew,
Thanks you for getting this out and about:
-------------------------- ai_sparc_image.xml --------------------------
line 657: Is auto-install-common not automatically pulled in by
auto-install? I see the auto-install package has:
depend
fmri=pkg:/system/install/auto-install/[email protected]
type=require
Or is there another reason for including it explicitly?
(Especially under the text-install heading?)
--------------------------- ai_x86_image.xml ---------------------------
line 516: I don't see gnu-tar in ai_sparc_image.xml but I do it in both
text_mode_sparc.xml and text_mode_x86.xml is the omission
intentional in ai_sparc_image or inclusion intentional here?
--------------------------- install-finish ----------------------------
line 34 & 79:
Line 34 says installer type is required but line 79 has it in
brackets ('['/']') which implys optional
line 115 (NIT): Stylistically, I'd prefer to see:
if INST_TYPE not in ('TEXT', 'AI', 'LIVE'):
I like this, as it is more compact and explicit of what
the acceptable values are; plus, the logic is more compact
-- no and's -- resulting in less logic to accidently be
broken in later bug fixes.
line 115 & 117: I belive if the -I is not passed in a ValueError will be
raised? Perhaps INST_TYPE should be set to NONE before
line 92 and appropriate checks done?
line 117 (NIT): * To be consistent with line 116 and 121 "Was passed[...]"
should be capitalized as "was passed[...]".
* Perhaps providing a clue of acceptable types would be
nice?
line 118: Insight to the development says this is a left over debug
statement...
line 232 (PEP8): Need a space after '#'
line 233 (PEP8): Need a space after '='
There is a program called pep8 installed on indiana-build; please use it!
(I will stop commenting on pep8 formatting issues which it should catch
from here on out.)
--------------------------- net-fs-root ---------------------------
lines 31-39 (NIT): The below might be text related but I think grouping
that stuff here is actually detrimental to my
understanding of what's going on and the way most
things like this are grouped across slim_source.
(Also such a comment is likely to become inaccurate
without notice as the file is modified in the future.)
Please group the SMF FMRI's together (and be consistent
with the _FRMI variable name across all FMRI's). The
SVCADM variable should be in with the other commands.
Each group should be alphabatized too.
line 154: Is this (set -- "") necessary?
line 156: This if statement is extraneous
line 157: while (( $# > 0 )) ; do
line 162: Wouldn't break be more appropriate here since we have an answer
why keep looping?
line 184: Please use something like:
if [[ "${INSTALL_FLAG}" == ~(E)^true* ]]
lines 341-349: Couldn't just the sun4u or sun4v boot_archive be downloaded
or does the sun4v need both?
lines 330-361: So, I'm admittedly very ignorant of what the text installer
expects but if you have any insights on why certain files
are required or why various links are could you comment
that?
line 363 (NIT): Please remove this comment
--------------------------- ict.py ---------------------------
line 1612: need a space after '#'
------------------ perform_slim_install.c. ------------------
This file needs c-style run against it (I think 'hg cstyle')
line 2333: Need a space after "="
line 2366: Need a space after last ","
On Tue, 31 Aug 2010, Chris Navrides wrote:
Hi all,
Could we please get a code review of the modifications made to enable 6971585
Network Text Install
This will enable a client over the network to either do an Automated Install or
a Text Install. The default in the grub menu will
lead to the text install menu. For sparc if no "-install" flag is set then it
will also default to the text-install menu.
Webrev:
http://cr.opensolaris.org/~chris_n/netboot_text_install/
We have done testing with both x86 and sparc using the test bed on the 10.10.45
subnet. There are no changes done to installadm on
the server side.
Thanks,
Chris & Andrew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
