Hi Keith. Huge thanks for taking the feedback!
See my comments on the finalizer scripts below. Gleen will comment on the other bits. Joe Keith Mitchell wrote: > Hi Glenn & Joe, > > There's been a lot of comments already - I tried to make sure my > comments weren't addressed, but if I missed something, please point me > to your other response. Thanks. > > DC-manifest.rng: > - With the changes for VMC, it seems <img_params> is an optional subset. > As you mentioned in another response, it's not *truly* optional. It > seems like originally you were going for something like the following > (based on the commented sections and your previous responses) > > <start> > <element name="distribution"> > <attribute name="name"/> > <ref name="nm_distro_constr_params"/> > <choice> > <ref name="nm_img_params" /> > <ref name="nm_vmc_params" /> > </choice> > </element> > </start> > > I'm curious why you reversed that reasoning; it seems like such a > restriction would allow us to enforce the need for img_params to exist > when building a live image, but also not cause the parser to fail on VMC > manifests. While there aren't any VMC specific parameters right now, > there could be in the future, and I don't think there's anything wrong > with an empty <vmc_params/> element in VMC manifests for now. > > DC-manifest.defval.xml: > The need to specify "skip_if_no_exist" on almost every field suggests to > me that there 'something' needs to change - likely in our defval > processing code, to increase flexibility. Off the top of my head, it > seems the defval manifests are distinctly NON-tree like, which is what > is causing the flexibility: "missing_parent" actions either create the > whole tree or not, regardless of how much of the tree is already there. > If we had a manifest, that, say, could look more like this: > > <default node="img_params" missing="skip"> > <default node="pkg_repo_default_authority" missing="skip"> > <default node="main" missing="create"> > <default node="url" missing="create" /> > <default node="authname" missing="create" /> > </default> > </default> > ... > > Then, we could evaluate each node, but only if we have a place to put it > already (because the parents are there). Then, we wouldn't really need > skip_if_no_exist littered throughout. This is just a quick example, not > necessarily the 'best' solution. If it's not in scope for VMC release, > then can you file an RFE to make an enhancement along these lines? > > im_pop.py: > General note: As I understand, this code was simply moved and isn't new. > So if any of my comments are truly out of scope changes, can you file bugs? > > General: > try: > ... > except: > ... > will catch more exceptions than intended, including KeyboardInterrupt > (a.k.a. control-C) and SystemExit. In general, per PEP 8, except clauses > should be as specific as possible when catching exceptions. If a generic > exceptions is *truly* needed, it should be "except Exception:" (which > will catch everything BUT KeyboardInterrupt and SystemExit). > > 34-37: Just want to reinforce the desire that these be made into import > statements. > 83-104: Python/C code nit: raise Exceptions (or subclasses thereof), > don't return status codes. Particularly given that below, the status > code is checked for non-zero and an exception is raised, the change here > should be simple. In other cases, I don't know how disruptive a change > this will be, depending on how many consumers of the function there are > and how equipped they are to handle a change from C-style status codes > to Python style try/except clauses. > --Same comment for other functions. Some are just strictly calling > tm_perform_transfer, so truly, the easiest way to affect such a change > would be to change *that* which I think is out of scope here. > > 227: There's no "if __name__ == '__main__':" clause to lead off the > "Main" section. > > 352: I think this should be "IOError"? > > ---- Shell scripts ---- > > create_vm: > 274, 284: Nitpick, but why not capture the output from the first call to > "VBoxManage -v"? Good point! I will change it to be invoked only once. > > 295-300: We blindly destroy an existing VM? Will first time users blow > away their VMs when they give their VMC build name the same name as a > current VM? Should we be using the --basefolder flag to create the VM in > the DC build area instead of $HOME/.VirtualBox? (Especially since we're > running as root to fire off DC...). If we did this, we might be able to > handle my previous comment by unregistering an existing VM if it's not > located in the DC build area (and of course, re-registering it when > finished). Yes agreed. We investigated using --basefolder but did not have the full success. We wanted to move forward and felt the likelihood of a user being negatively impacted was small. We will use --basefolder in phase II. > > 341: Only 32 bit Solaris? Yes agreed. Again We wanted to move forward. 64 bit option could be added in Phase II. > 341-343: In the case of the hardcoded parameters, can we set variables > at the top of the script, and reference them here? This would make it > easier for users to modify if they truly wanted (and easier for us to > add flexibility in the future) Good point. Will do. > > export_esx: > 218-232: This is duplicate code from elsewhere. Any chance we can > isolate this somehow? Good point. Others have commented on this also. I will move this repeated code into a common function. > > 285: Am I confused, or is this backwards? You create the directory, then > immediately rm -rf it? Good catch. It's backwards! > > export_ovf: > 218-232: Same comment as export_esx > 285: Same confusion! Same answers. ;) > > General note: Someone else mentioned that instead of VBoxManage, you > should set a variable to the precise path and use that. I'd recommend > setting that variable to have the '-q' flag embedded, since it's used in > each case. Will do. > > Thanks, > Keith Thank you! Joe > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
