Hi Karen, responses inline
On Fri, Sep 10, 2010 at 4:40 PM, Karen Tung <[email protected]> wrote: > Hi Chris and Andrew, > > My comments are inline. > > > On 09/ 9/10 04:05 PM, Chris Navrides wrote: > > Thank you all for the comments. We have updated all of your concerns that > you mentioned into our new webrev: > http://cr.opensolaris.org/~chris_n/netboot_text_install_v2/<http://cr.opensolaris.org/%7Echris_n/netboot_text_install_v2/> > > and the updated parts located at: > http://cr.opensolaris.org/~chris_n/netboot_text_install_part/<http://cr.opensolaris.org/%7Echris_n/netboot_text_install_part/> > > To address specific concerns: > > - ict.py: The check was added to give additional debugging statements. > Install finish was also now modified so that the packages to remove are the > exact same for text and auto install. This check will prevent an error > later > down from trying to uninstall a package that > > ^^^^^^^^^^ > I think you meant "slim install" and "text install" there? > yes we did. > > - is not there. > > Personally, I don't like the changes in install-finish to not exactly > specify what to remove per image type. > IMO, it is much more clear to explicitly specify what packages you want to > remove for which image type, > and make sure that those packages are indeed removed. If we expect those > packages to exist for > an image type and it doesn't exists for some reason, something is very > wrong, and this change will > make it harder to debug those kind of problems. > The packages that are being removed are just the installers. If these are not there then this script will not run anyways. > > > > > > - We made modifications to installadm and to the finalizer script that > generates .image_info so that if it is a net-text image installadm would > generate a new GRUB menu to represent that the default choice is now text > install. We were looking for feedback if this is the right way to go. If we > change the GRUB menu some users may get confused when they boot it up for > the sole purpose of getting dropped into a console and see <release> Text > Install. However a user who wants to use it to do a text install may also > get confused if it doesn't say anything except <release> boot image. Does > anybody have an alternative or feel strongly one way or the other? > > It took me quiet a while to figure out what you meant above. After > reading the code and talking with Keith, > I think you want the person that builds the image to be able to control > whether > the words "text installer" is displayed in the grub menu or not. > > Since you are changing the AI manifests to include the text installer > functionality in the image at all times, I think it will be even more > confusing to allow users to > control what gets displayed in the grub menu. > > I understand your concern about using the "fail safe" option of the grub > menu entry to invoke the > installer might be confusing to users. What about presenting an > additional entry > in the grub menu for invoking the text-installer? installadm can be > updated to check whether a certain > image contains the text installer, if so, add that entry. If not, don't > add that entry. Leave the original > fail safe entry to behave as it use to. > I believe you have talked with Keith about this. > > Since there's really no agreement on what to do with the grub menu changes > yet, I didn't review > those changes. My comment below are for the other changes: > > > usr/src/cmd/distro_const/auto_install/ai_sparc_image.xml > > - Why do you add the "gen_cd_content" finalizer script before > the "post_boot_archive_pkg_image_mod" finalizer script? > The "gen_cd_content" finalizer script should be one of the > last finalizer scripts to run because it needs to "record" > all the files in the CD. "post_boot_archive_pkg_image_mod" > script moves all the files around to create the zlibs. So, > so, the gen_cd_content script will not generate the correct > list of files if you run before it. > This was a mistake, we only changed it in the x86 version. > > > usr/src/cmd/slim-install/svc/net-fs-root > > - line 329-346: this is still not correct. You are doing wget for BOTH > boot archives. > The idea is that you should detect which whether you are booted 64 bit or > 32 bit. > For the one that you are booted with, no need to re-download the archive. > Just create the > symlink as appropriate. Then, download the archive one you are not booted > with, and create the > link. > Since the boot archive we booted from is not stored on the client we cannot just change the link. Since we need both to install we need to re-download it. > > - lines 348-368, for sparc, I don't think you need to download the boot > archives > again since you are already running it. Perhaps you just need to adjust > the link. > Same. > > - line 371: Without a valid .livecd-cd-content file, the install might > appear to work, > but it doesn't guarantee to install only what you expect it to. Please > generate one and > download it in this script. > Again, I understand you spoke with Keith about this. > Thanks, > > --Karen > > > > We would appreciate any additional feedback. > > Thanks, > Chris & Andrew > > > > > On Tue, Aug 31, 2010 at 1:35 PM, Chris Navrides <[email protected]>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_instal<http://cr.opensolaris.org/%7Echris_n/netboot_text_install/> >> l/ <http://cr.opensolaris.org/%7Echris_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 > > >
_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
