Tom, This looks good in general.
I think that we need to throw an exception if the Architecture and CPU are not supported. The exception can be caught where the checkCPU and checkArch are called if we want to just pass on the error. But if the exception is added then when we add a new version for either of those then the osol_install.auto_install.installadm_common will also be updated. Thus this part of the code will not need to be updated anyhow. The wording looks fine. Also what sort of test enhancements need to be made? Thanks, John On Jan 3, 2012, at 7:43 AM, Tomas Dzik wrote: > Hi all, > I would like to ask you for a code review for a bug > > 7015427 - installadm does not validate criteria values > > Webrev: > > https://cr.opensolaris.org/action/browse/caiman/t.dzik/7015427/ > > Couple of questions: > > 1) Could you please check the wording of Warnings I added ? > > 2) Is it OK just to print warnings or should I rise the exception ? > I decided just to print warning because in such case my changes will not > prevent installation of the potential new architectures but I can change it > if you feel that Error is more appropriate here. > > Testing: > > 1) Sources are pep8 clean. > 2) Tests passed > 3) I tried to create manifest and profile with criteria cpu and arch, using > allowed and not-allowed cpus and archs, using single value and lists of value > and I checked that proper warnings are printed and criteria updated. > 4) I tried also installadm set-criteria command. > 5) I tried to set criteria using also .xml file > > Best regards, > > Tomas D. > _______________________________________________ > 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
