On 11/29, David Lutterkort wrote:
> 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 ?
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 <[email protected]>
Deltacloud API, CloudForms
