Hi Karen, The updated webrev can be found here: http://cr.opensolaris.org/~joev/bug15163_r2/
My comments are below. Thank you for the review. Joe On 03/12/10 07:41 PM, Karen Tung wrote: > Hi Joe, > > I have a couple of general comments: > > 1) you said bug 15163 is used to track ALL the work that is needed for > DC for the AI Image management project. Is the changes in this webrev > all the changes that are needed? If not, does this mean that you will > not push these changes even if the code review is approved? This webrev represents the changes needed to the DC for the AI Image management project. I do plan to push these changes as soon as the webrev has been approved. > > 2) Can you describe how you test these changes? I built the D-C package in the slim_gate. I installed this new package on a test machine. I built an ai image, along with the pkg(5) repo with the new ai-publish-pkg checkpoint. To confirm the ai-publish-pkg checkpoint worked I started a pkg.depotd server using the results of that checkpoint. I also have done full and partial D-C runs resuming at various checkpoint to confirm the desired TMP directory cleanup. See caiman-discuss email thread with subject: "Does the DC finalizer clean the temp directories between checkpoints by design?" > > > Here are my comments about the code: > > ai_sparc_image.xml: > ai_x86_image.xml: > ---------------------------------- > - What is "arg_name=" in the second line of the > comments. I didn't see it mentioned else where. > Do you mean all the 3 things in the "argslist"? I have fixed these comments. > > - If the previous assumption is correct, I don't think > it is a good idea to require people to specify arguments that > they are not going to specify values for. The software > should be made smart enough to not have that requirement. The software has been updated to allow these arguments to be optional. > > ai_publish_pkg: > --------------------------- > - lines 125-142: The cleanup() function does not remove the potentially > partially populated repository. Should a non-complete repository be > left around? fixed > > - line 244-303: All these code makes the assumption that there are 8 > arguments > that will be passed in, and just references them. It would be important > to check to make sure that there are actually 8 arguments that you can > access before accessing any of them. I had actually been doing that in the original webrev. See lines 231->236 in the original webrev: http://cr.opensolaris.org/~joev/bug15163/usr/src/cmd/distro_const/auto_install/ai_publish_pkg.html You must have just missed that. It is done in the updated webrev on lines 238->244 > > - line 244-303: The "invalid_argument_found" variable is defined. I > don't think > it is necessary. Since any invalid argument will cause the program to exit. > Why not exit immediately? Why waste time to do other computations? This method will prevent the user from running multiple times to identify all invalid arguments. For example, in the worse case, if all 8 required and optional arguments are bad, this method prevents the user from running once, being notified the 1st argument is bad, rerunning again to be notified the next argument is bad... and so forth. This method notifies the user of all invalid arguments in a single run. > > - line 251-267: I think all these code is not necessary because those > values > are not used for installation at all. Even though this script does not use them today if any of them are invalid it could indicate a seriously wrong environment. Also if a future code change requires more of these arguments they are already checked. > > - line 370: why is this statement inside $(), while all the other > similar pkgsend > commands don't have this? It's the way it work. The output of the command is a string containing a command which must be run. > > - line 389-391: Since this is a finalizer script, and not a normal user > program, I don't > think it is necessary to print this kind of information. People who > decide to use > this finalizer script should know how the repo can be used. I have removed this output. > > Thanks, > > --Karen > > On 03/12/10 12:52 PM, Joseph J VLcek wrote: >> Please review the fixes for: >> >> Bug 15163 - DC needs to be enhanced to publish the AI image as an IPS >> repository >> >> The webrev is here: >> http://cr.opensolaris.org/~joev/bug15163/ >> >> Bug 15163 will be used to track the work required to update the DC to >> meet the needs of project "Automated Installation Service and Image >> Management". >> >> I have tested these changes on x86 and will test on SPARC before pushing. >> >> I have developed the new finalizer script, ai_publish_pkg, in ksh. The >> IPS command used by this script, pkgsend(1), does not have a direct >> Python interface. However the IPS Transaction class could be used to >> replicate what the pkgsend(1) command does allowing this script to be >> implemented in Python. In discussing this with Dave Miner he wasn't >> all that concerned that this script stay ksh. >> >> Thank you. >> >> Joe >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > Thank you. Joe
