Thanks, Mark, for the detailed feedback. Some comments inline ...
On Mon, 2010-04-26 at 22:12 +0100, Mark McLoughlin wrote:
as we discussed earlier it probably won't be usable as a
tool to run from shell scripts e.g.
#!/bin/bash
for vm in $(rhevmadmin vms:list); do
rhevmadmin vms:start $vm
done
as the shell itself is so heavyweight. That probably means it only makes
sense as an interactive admin console.
Yeah, it really would be just for interactive use.
That said however, it could be made significantly less weighty by
providing a more minimal deployment based on bare karaf+cxf-jaxrs
instead of full-on servicemix (which literally throws in the kitchen
sink!)
Also the shell does support some control structures & closures, though
the syntax is a tad unconventional. For example, your for loop above
would be expressed _within_ the shell with something like:
vmsByName = { a=[0] ; each (vms:list) { $a get $0 } } ; vms:start
(vmsByName)
The above is just psuedocode, as I'd need to look into the exact syntax,
but there are some magic incantations available all right.
Also, we might be setting ourselves up for a bit of quagmire (given
the
dependencies) if/when we decide to package it as an RPM.
Yeah, I need to review that info on multiplatform deployment to get a
feel for how quagrimey it could get.
- the api package is now a 'bundle' ... so, a bundle is
just a jar
with some extra stuff in MANIFEST.MF ... what would happen if we
wanted to add some other metadata to the manifest? Just a
theoretical question, but it seems a little wrong that our core
would be a bundle just for the sake of the cli.
Well the general principal is that conversion to a bundle should be
non-intrusive. OK the manifest has been decorated with some extra info,
but these extra manifest headers are simply ignored anytime the bundle
is used as a plain ol' jar, e.g. within a webapp WEB-INF/lib.
So really the jar becomes a bit more useful, in the sense that it may
now be deployed directly to OGSi, without really impacting on any
existing usage scenario.
That said however, I could always just wrap the api jar as a dynamic
bundle (same approach as I took with the RESTeasy provider jars) if
you'd be more comfortable with that?
- features.xml confused me for a minute:
+<features>
+ <feature name="rhevm-api-cli">
+ <feature>cxf-jaxrs</feature>
+
<bundle>wrap:mvn:org.jboss.resteasy/resteasy-jaxb-provider/2.0-beta-2</bundle>
+
<bundle>wrap:mvn:org.jboss.resteasy/resteasy-jettison-provider/2.0-beta-2</bundle>
+ <bundle>mvn:com.redhat.rhevm.api/rhevm-api/0.1-SNAPSHOT</bundle>
+
<bundle>mvn:com.redhat.rhevm.api/rhevm-api-cli-vms/0.1-SNAPSHOT</bundle>
+ </feature>
+</features>
that means that the rhevm-api-cli feature depends on the cxf-jaxrs
feature (we're using CXF's REST client here), the jettison/jaxb
providers (because the api jar requires them) and then the two
bundles for the cli itself. Right?
Yep, right on the money.
Features are really just an aggregation mechanism. Already I've added
another two bundles (rhevm-api-cli-base & rhevm-api-cli-hosts) and will
be adding more (datacentres etc.) so it would become a right PITA to
individually install each bundle. So the feature provides a kinda
short-hand for [un]installation in one fell swoop.
- can we do better error handling here:
+ protected void doAction(String action) throws Exception {
...
+ System.err.println("VM[" + name + "] not found.");
Yep, I'll look at improving that.
- the api jar has a Link header parser you could use
Do you mean the Link.valueOf() method? Yeah, I switched over to using
that yesterday in the commit to concatenate the separate link header
values.
- This looks dubious to me:
+ try {
+ VMs vms = vmClient.getVMs();
+ for (VM vm : vms.getVMs()) {
+ delegate.getStrings().add(vm.getName());
+ }
+ } catch (Exception e) {
+ // Ignore
+ }
what exception are you ignoring here exactly?
Yeah that's sloppy, good catch. What was probably going through my mind
was that command completion shouldn't expose errors, being simply a
convenience that's tangential to main thrust of the task. So if the
vmClient.getVMs() failed with a IOException or summat, I wouldn't
necessarily want to expose it at that point.
I've since factored out that getVMs logic to a common base client with
sig:
public <S> S getCollection(String rel, Class<S> clz) throws Exception
I'll look into a better error handling strategy for that method. It may
make more sense to remove the throws clause from the signature and
instead describe any error on stderr within that method, as opposed to
bubbling it back to the caller:
VMs vms = client.getCollection("vms", VMs.class);
- In the toplevel POM, felix.karaf.version isn't needed, but it
might
be nice to have a property for the maven-bundle-plugin version
I've re-org'd the dependencies a bit, to keep all the
version-constrained dependencies in the top-level pom (as was the
convention).
Fair point on the maven-bundle-plugin version, I'll change that now.
Cheers,
Eoghan