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"?
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).
341: Only 32 bit Solaris?
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)
export_esx:
218-232: This is duplicate code from elsewhere. Any chance we can
isolate this somehow?
285: Am I confused, or is this backwards? You create the directory, then
immediately rm -rf it?
export_ovf:
218-232: Same comment as export_esx
285: Same confusion!
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.
Thanks,
Keith
