deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 3/4] CIMI: Initial inclusion of CIMI action models
Date Thu, 07 Feb 2013 23:51:26 GMT
On Thu, 2013-02-07 at 13:52 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> The way of how we currently create CIMI entities is not
> really nice. We parse the raw JSON/XML and then we are
> trying to create the CIMI entity.
> 
> Besides the code looks ugly, there is no way to add validation
> for required attributes without make it even more uglier.
> 
> This patch introduce the 'models/actions/' directory where
> we can store the CIMI 'actions' model, like 'MachineTemplateCreate'
> model.
> 
> Then in MachineTemplate model, we can leverage the '.from_xml' and
> the '.from_json' methods to create the action model and then use it
> to create the CIMI entity (MachineTemplate in this case)

Definitely an improvement; a couple things:

      * I don't think we need a separate subdirectory for these
        (especially since the subdir is not reflected in the class
        name ;)
      * If you set up the MachineTemplateCreate in the server with
        MachineTemplateCreate.parse(request.body, request.content_type)
        you can get rid of create_from_json/create_from_xml
      * I would make create a method on MachineTemplateCreate. Seems
        more logical to me

> diff --git a/server/lib/cimi/models/machine_template.rb b/server/lib/cimi/models/machine_template.rb
> index 5ba9b3e..c8fa631 100644
> --- a/server/lib/cimi/models/machine_template.rb
> +++ b/server/lib/cimi/models/machine_template.rb
> @@ -52,28 +52,27 @@ class CIMI::Model::MachineTemplate < CIMI::Model::Base
>        end
>      end
>  
> -    def create_from_json(body, context)
> -      json = JSON.parse(body)
> +    def create(template, context)
>        new_template = current_db.add_machine_template(
> -        :name => json['name'],
> -        :description => json['description'],
> -        :machine_config => json['machineConfig']['href'],
> -        :machine_image => json['machineImage']['href'],
> -        :ent_properties => json['properties'] ? json['properties'].to_json : {}
> +        :name => template.name,
> +        :description => template.description,
> +        :machine_config => template.machine_config.href,
> +        :machine_image => template.machine_image.href,
> +        :ent_properties => template.property.to_json

My DB patches add a Entyt#properties which handles
serializing/deserializing into ent_properties. We need to get machine
templates and similar objects to use the same mechanisms.

It would be great if we could drive the copying of attributes to/from
Entity and model objects off some introspection (e.g., sync all
attributes that Entity and the model object have in common, except for
id)

> @@ -92,7 +91,10 @@ class CIMI::Model::MachineTemplate < CIMI::Model::Base
>          :property => (model.ent_properties ? JSON::parse(model.ent_properties) :
 nil),
>          :created => Time.parse(model.created_at.to_s).xmlschema,
>          :operations => [
> -          { :href => context.destroy_machine_template_url(model.id), :rel => 'http://schemas.dmtf.org/cimi/1/action/delete'
}
> +          {
> +            :href => context.destroy_machine_template_url(model.id),
> +            :rel => 'http://schemas.dmtf.org/cimi/1/action/delete'
> +          }

This hunk is only formatting changes, right ?

David



Mime
View raw message