deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH core 1/6] CIMI: Initial support for persisting attributes in database
Date Fri, 30 Nov 2012 09:10:34 GMT
On 11/29, David Lutterkort wrote:
> Hi Michal,
> 
> ACK. Some questions:
> 
> On Thu, 2012-11-29 at 14:05 +0100, mfojtik@redhat.com wrote:
> > diff --git a/server/config.ru b/server/config.ru
> > index efed62f..406c769 100644
> > --- a/server/config.ru
> > +++ b/server/config.ru
> > @@ -54,6 +54,7 @@ end
> >  #       different root_url's
> >  #
> >  frontends.each do |frontend|
> > +  frontend = frontend.strip
> 
> This should go into a separate patch - presumably it's to fix user
> typos ?

Right. Sorry for that, I really need to get more familiar with git add :/

> > diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> > index 6a73cf6..a5d8f34 100644
> > --- a/server/lib/cimi/models/base.rb
> > +++ b/server/lib/cimi/models/base.rb
> > @@ -285,4 +285,14 @@ class CIMI::Model::Base < CIMI::Model::Resource
> >      end
> >      self.class.new(attrs)
> >    end
> > +
> > +  class << self
> > +    def store_attributes_for(context, entity, attrs={})
> > +      stored_attributes = {}
> > +      stored_attributes[:description] = attrs['description'] if attrs['description']
> > +      stored_attributes[:name] = attrs['name'] if attrs['name']
> > +      stored_attributes[:ent_properties] = attrs['properties'].to_json if attrs['properties']
> > +      context.store_attributes_for(entity, stored_attributes)
> 
> Why do we need the context here at all ? If we store in the database on
> the level of CIMI model objects, rather than backend (driver) objects
> there's no need for that. I don't like the idea that we need to pass the
> context/driver this deep down, and that the responsibility for
> storing/retrieving to/from the DB is distributed this widely (e.g., in
> Machine.delete!).

Good point thanks! I used context to generate 'url' in one of the previous
revisions. Currently there is no point in doing that. I will remove this
right after DMTF meeting :-)

> Please don't address this comment just yet, we'll do that after the DMTF
> meeting.
> 
> > @@ -120,10 +129,16 @@ class CIMI::Model::Machine < CIMI::Model::Base
> >    def self.from_instance(instance, context)
> >      cpu =  memory = (instance.instance_profile.id == "opaque")? "n/a" : nil
> >      machine_conf = CIMI::Model::MachineConfiguration.find(instance.instance_profile.name,
context)
> > +    stored_attributes = context.load_attributes_for(instance)
> > +    if stored_attributes[:property]
> > +      stored_attributes[:property].merge!(convert_instance_properties(instance,
context))
> > +    else
> > +      stored_attributes[:property] = convert_instance_properties(instance, context)
> > +    end
> 
> How about
> 
>         stored_attributes = context.load_attributes_for(instance) || {}
>         stored_attributes[:property].merge!(convert_instance_properties(instance, context))

ACK. Will add fix it.

> > @@ -131,8 +146,8 @@ class CIMI::Model::Machine < CIMI::Model::Base
> >        :disks => { :href => context.machine_url(instance.id)+"/disks"},
> >        :volumes => { :href=>context.machine_url(instance.id)+"/volumes"},
> >        :operations => convert_instance_actions(instance, context),
> > -      :property => convert_instance_properties(instance, context)
> > -    }
> > +      :property => stored_attributes
> > +    }.merge(stored_attributes)
> 
> The ':property => ... ' line doesn't seem to belong there. The :merge
> should already do the right thing.
> 
> > diff --git a/server/lib/db.rb b/server/lib/db.rb
> > new file mode 100644
> > index 0000000..e0f28d4
> > --- /dev/null
> > +++ b/server/lib/db.rb
> > @@ -0,0 +1,55 @@
> > +module Deltacloud
> > +
> > +  require 'data_mapper'
> > +
> > +  require_relative './db/provider'
> > +  require_relative './db/entity'
> > +
> > +  DATABASE_LOCATION = ENV['DATABASE_LOCATION'] || "/var/tmp/deltacloud-mock-#{ENV['USER']}/db.sqlite"
> > +
> > +  def self.initialize_database
> > +    DataMapper::Logger.new($stdout, :debug)
> > +    DataMapper::setup(:default, "sqlite://#{DATABASE_LOCATION}")
> > +    DataMapper::finalize
> > +    DataMapper::auto_upgrade!
> > +  end
> > +
> > +  module Helpers
> > +    module Database
> 
> Why doesn't this live in lib/deltacloud/helpers ?

I tried to separate all database stuff into one file, but you're right this
need to go to lib/cimi/helpers.

> 
> > +      include Deltacloud::Database
> 
> How do I find that when I first look at the code ? (It's not in
> lib/deltacloud/database.rb)

This is here just because I don't want to write Deltacloud::Database::Entity,
but just Entity. We can safely remove that.

> 
> > +      def current_provider
> > +        Thread.current[:provider] || ENV['API_PROVIDER'] || 'default'
> > +      end
> 
> Ugh .. this introduces nasty dependencies on how we are running things.
> It might be time to write the driver/provider into every
> Deltacloud::Model (or CIMI::Model) object.

What you mean with nasty dependencies? We use this in views too
(api.xml.haml, etc). I think the best place for this method will be
'drivers.rb.

> 
> Again, not something to fix right now, but ultimately, I'd like to get
> to a point where one set of our model objects cleanly encapsulate
> persistence.
> 
> David
> 
> 

-- 
Michal Fojtik <mfojtik@redhat.com>
Deltacloud API, CloudForms

Mime
View raw message