Hi Clay, Thanks for the code review. Please see my responses inline.
Clay Baenziger wrote: > Hi Karen, > As I see it, I think this is an okay interim fix but the final > resolution should be implementing a postinstall action using Jan's > framework from bug 1040 to flip this setting in on the installed system > only. > That way downloading a package while running from the LiveCD won't > require using a lot more memory space to hold cache files which won't be > needed for a LiveCD environment, and the file can still get updated for > the installed environment. > > You raised a good point about using RAM to hold the cache file while the LiveCD is run if I turn the download cache back on after DC downloads the packages. I am going to leave download cache purge on in the LiveCD. I will modify the finisher script to turn the download cache feature back on after installation. > As far the code review: > > Lines 57-59: it seems the sed(1) line is more complex than necessary, but > I'm not as familiar with the /var/pkg/cfg_cache file as I'd like. Why not: > sed 's/flush-content-cache/flush-content-cache-on-success = True/' > $cfg_file > $tmp_cfg ? > This sed statement not work, it will only replace the "flush-content-cache" part of the string with the whole "flush-content-cache-on-success = True" string. > Line 63 why not set $status and return that rather than whatever "pkg > image-create" returned on line 38? > Yes, I should do that. I will fix it. > Lines 66-67 why not consolidate as a mv(1) like 'mv $tmp_cfg > $cfg_file' and then check its status too like lines 61-64? > Consolidate as a mv(1) and check the status also sound good. > Clay Baenziger wrote: > >> D'oh I see I missed lot's o' folks input already for the actual code >> review and my sed should probably have been: "sed >> 's/^flush-content-cache$/flush-content-cache-on-success = True/'" or >> whatever the full string is as Sundar said. This sed statement will not work either, because ^flush-content-cache$ will not match the string. If we change the string to ^flush-content-cache.* it will work. Thanks, --Karen
