Hi Michal, ACK. Some questions:
On Thu, 2012-11-29 at 14:05 +0100, [email protected] 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 ? > 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!). 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)) > @@ -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 ? > + include Deltacloud::Database How do I find that when I first look at the code ? (It's not in lib/deltacloud/database.rb) > + 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. 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
