LGTM now thanks, Darren.
On 26/03/2012 20:57, Matt Keenan wrote: > Darren, > > Thanks for the review, new webrev just uploaded, other comments below : > > https://cr.opensolaris.org/action/browse/caiman/mattman/7133369/bug-7133369/ > > On 23/03/2012 12:01, Darren Kenny wrote: >> Hi Matt, >> >> The general idea of implementation like this is fine. >> >> I wonder if it's worth a comment about the lack of HTTPS here, as in why it >> can't be done at this point? >> > > I had already logged RFE : > http://monaco.us.oracle.com/detail.jsf?cr=7151817 > > To ensure this was logged somewhere for future implementation. For > https/ftps support the SSL key/cert need to be available and they are > currently not available in the manifest-locator script. > >> In general, it's probably should quote the variables everywhere to avoid >> incorrect parsing - especially where the value of the variable is being >> input by a user, e.g. anywhere that $AI_MANIFEST_LOCATION is used, but no >> harm to do for most others either. > > Done > >> >> Lines 372-373, the tabbing seems wrong here... > Tab vs 4 spaces, fixed. > > cheers > > Matt >> >> Thanks, >> >> Darren. >> >> >> >> >> On 23/03/2012 10:17, Matt Keenan wrote: >>> Hi, >>> >>> Can I get a code review for the following AI RFE : >>> >>> Bug : >>> 7133369 Auto Installer should accept a specified manifest file using >>> a URL as input >>> http://monaco.us.oracle.com/detail.jsf?cr=7133369 >>> >>> Webrev: >>> >>> https://cr.opensolaris.org/action/browse/caiman/mattman/7133369/bug-7133369/ >>> >>> >>> Allow for the specification of a URI via AI boot argument "aimanifest". >>> Currently this boot argument can be set to nothing or "prompt". This RFE >>> extends this boot argument to allow it to contain a URI that points at a >>> file to use as the manifest for installation. >>> Valid URI's are http, ftp, tftp, file. >>> >>> Testing: >>> - build AI ISO and verified files are retrieved and install proceeds as >>> expected using the retrieved manifest. >>> - Ran full set of unit tests and no regressions found. >>> _______________________________________________ >>> 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
