On 07/13/2011 05:20 PM, Martyn Taylor wrote:
Chris,
I agree,
However, from what I can see (I may be proved wrong) is that there is not a specific
error message returned via the factory.
For this reason I chose to adopt the approach of validating the xml before we sent the
request. Of course this has its benefits too. Since it reduces the need to send a
request over the network and reduces the time taken for a response to the user.
The actual RNG is simply lifted from OZ so the only overhead here is keeping them in
sync. Which shouldn't be anything major? Maybe in the future when we have various
error codes etc... returned from factory we can update aeolus-image to do away with the
validation and just return a response to the user based on the error.
I'm inclined to go along with this -- at least until we have the error
reporting mechanism in the factory.
However, we do run into the risk of someone going in and changing the
schema file here in aeolus-image without realizing that it should be
synced with Oz.
So how about this: we put a comment on top of the aeolus-image schema
saying something like:
"This comes from oz/docs/tdl.rng; commit a1b2c3d4e5...; don't put
changes here but into the upstream."
Anyway, the code works, so ACK unless someone has any objections.
Thomas
Cheers
Martyn
----- Original Message -----
From: "Chris Lalancette"<clalance(a)redhat.com>
To: mtaylor(a)redhat.com
Cc: aeolus-devel(a)fedorahosted.org
Sent: Wednesday, 13 July, 2011 3:31:57 PM
Subject: Re: [PATCH aeolus] BZ#720406 Added validate xml against template schema check
On 07/13/11 - 01:01:53PM, mtaylor(a)redhat.com wrote:
> From: Martyn Taylor<mtaylor(a)redhat.com>
>
> ---
> .../image_factory/aeolus-image/examples/tdl.rng | 206 ++++++++++++++++++++
> .../image_factory/aeolus-image/lib/base_command.rb | 6 +
> .../aeolus-image/lib/build_command.rb | 10 +
> .../aeolus-image/spec/base_command_spec.rb | 14 ++
> .../aeolus-image/spec/build_command_spec.rb | 11 +
> .../spec/fixtures/invalid_template.tdl | 18 ++
> .../aeolus-image/spec/fixtures/valid_template.tdl | 18 ++
> 7 files changed, 283 insertions(+), 0 deletions(-)
> create mode 100644 services/image_factory/aeolus-image/examples/tdl.rng
I'm of two minds about this.
On the one hand, it is nice to give the user feedback right away if their TDL
is invalid. On the other hand, oz/imagefactory already does this checking.
The duplication means that if we add a new feature to the TDL (which we do
rather frequently), we have to update both oz/imagefactory and this RNG to
allow it through. It makes us less nimble.
I think I would prefer a way to get error messages from the imagefactory
printed out on the aeolus-image command-line. Has anyone taken a look at
doing it this way instead?