Hi Ethan,
Ethan Quach wrote: > Jan, > > Thanks for the review... > > > jan damborsky wrote: >> Hi Ethan, >> >> please see my comments below. >> >> Thank you, >> Jan >> >> >> I have general comment for DC changes. Since installadm(1M) >> tools will take care of populating auto_install directory if it >> doesn't exist, does DC need to take care of it as well ? > > The original intent was to do it only in DC so that installadm > has to do less work, but then I realized to support older build > images, installadm should also do it as a back up, since older > images won't have the directory there. I see - thank you for clarifying this. > > So the fact that installadm does it was supposed to be > secondary. In the future, if we change our scripting to just > loopback mount the AI iso instead of cpio'ing it, (an idea we've > thrown around before), it'd be better that these images have > them there since the imagepath won't be writable, and we'd > still want this support for those images. This is a good point - that will mean that installadm part which takes care of populating auto_install subdirectory would be removed at that point, since no writes will be possible to AI image area - might it be valid assumption ? > > > The alternative would be to just copy off the needed files > away from the imagepath altogether, into some per-service > configuration location on the server. But if we every wanted > to change to do this, it wouldn't be incompatible with either > change that we do now. I see - I think that having this stuff as part of AI image is better at this point, since schema to be picked up for validation is determined directly by AI image to be booted and indirectly by install service. That means if different AI image was associated with given install service, it wouldn't be needed to replace that per-service configuration location with files from newly associated AI image. > >> > >> >> setup-image.sh >> -------------- >> >> generic comment - stderr of CLI used is redirected to /dev/null >> (e.g. 310, 327, ...). >> I am thinking if it might be better it goes to the console, >> since if something goes wrong when those commands are invoked >> it might be easier to debug potential issue. > > It's kinda moot. These scripts are all exec'ed from installadm.c > so none of these are currently seen anyway. But you're right, > it probably makes sense for some places to allow stderr to go > to the console, but not all. I'll update the calls at lines 317, 327, > 334, 345. That sounds reasonable? Why not to allow stderr going to console for other commands ? I mean lines 310, 331, 335, 343, 347. > > One thing I'm failing to capture is the return code at line 340. > I'll add this. Also, I'm going to redirect 340 to /dev/null since > we don't really want to see that ugly "records in/records out" > output. (If we ever call these scripts directly) These are good suggestions - thanks for making those changes ! Jan
