deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Koper, Dies" <di...@fast.au.fujitsu.com>
Subject RE: [PATCH] CIMI: system and system_template support for mock driver, with unit tests. Just GET for now, no subcollections
Date Wed, 20 Feb 2013 05:39:29 GMT
Hi David,

> > +    set :capability, lambda { |t| true }
> 
> Is this intentional/still needed to avoid the problems that you were
> seeing ? If it's meant to stay here, it needs a big FIXME comment;
> without a proper capability check, system_templates will be advertised
> for any driver.

I'll change it to check for the system_templates operation in the driver.
I think my idea was that system_templates would be supported for all drivers using the DB,
but as I haven't included any DB support, I'll change it.

> > +    set :capability, lambda { |m| driver.respond_to? m }
> 
> IIRC, this is the capability check that was giving you trouble - is this
> working now for you ?

Yes it was. It is still not working for me. Still hoping Michal will look at it and figure
it out.

> This test failed for me because NS is not declared here; you can safely

Interestingly, the first time I had NS declared here and it failed because it was declared
(somewhere?) already, so I deleted it.
I'll check it again.

> You should add some tests though that do some SystemTemplate-specific
> manipulations and test that that works out.

I don't think there's any support for manipulations yet (just GET), but will add as I add/test
more actions.
As I haven't included any subcollections yet, there are also no SystemTemplate specific attributes
to test.

> Same comment as previous test. Again, it would be good to add some tests
> that test System-specific things, even if it's only the presence of
> attributes that only Systems have.

I can test system state, I'll add that.

Cheers!
Dies Koper


> -----Original Message-----
> From: David Lutterkort [mailto:lutter@redhat.com]
> Sent: Wednesday, 20 February 2013 4:20 PM
> To: dev@deltacloud.apache.org
> Subject: Re: [PATCH] CIMI: system and system_template support for mock
> driver, with unit tests. Just GET for now, no subcollections
> 
> On Tue, 2013-02-19 at 13:59 +1100, diesk@fast.au.fujitsu.com wrote:
> > From: Dies Koper <diesk@fast.au.fujitsu.com>
> 
> ACK, with a few comments and suggestions for changes before commit:
> 
> > diff --git a/server/lib/cimi/collections/system_templates.rb
> b/server/lib/cimi/collections/system_templates.rb
> > new file mode 100644
> > index 0000000..88fbe30
> > --- /dev/null
> > +++ b/server/lib/cimi/collections/system_templates.rb
> > @@ -0,0 +1,72 @@
> ...
> > +module CIMI::Collections
> > +  class SystemTemplates < Base
> > +
> > +    set :capability, lambda { |t| true }
> 
> Is this intentional/still needed to avoid the problems that you were
> seeing ? If it's meant to stay here, it needs a big FIXME comment;
> without a proper capability check, system_templates will be advertised
> for any driver.
> 
> > diff --git a/server/lib/cimi/collections/systems.rb
> b/server/lib/cimi/collections/systems.rb
> > new file mode 100644
> > index 0000000..42fbd06
> > --- /dev/null
> > +++ b/server/lib/cimi/collections/systems.rb
> ...
> > +module CIMI::Collections
> > +  class Systems < Base
> > +
> > +    set :capability, lambda { |m| driver.respond_to? m }
> 
> IIRC, this is the capability check that was giving you trouble - is this
> working now for you ?
> 
> > +      action :stop, :with_capability => :stop_system do
> > +        description "Stop specific system."
> > +        param :id,          :string,    :required
> > +        control do
> > +          system = System.find(params[:id], self)
> > +          if grab_content_type(request.content_type, request.body)
> == :json
> > +            action = Action.from_json(request.body.read)
> > +          else
> > +            action = Action.from_xml(request.body.read)
> > +          end
> 
> This whole if statement can be shortened to
> 
>                 action = Action.parse(request.body,
>                 request.content_type)
> 
> (similar for the other actions)
> 
> > diff --git a/server/tests/cimi/collections/system_templates_test.rb
> b/server/tests/cimi/collections/system_templates_test.rb
> > new file mode 100644
> > index 0000000..a4727ef
> > --- /dev/null
> > +++ b/server/tests/cimi/collections/system_templates_test.rb
> 
> This test failed for me because NS is not declared here; you can safely
> get rid of the $select, $filter, and $expand tests since they test
> generic collection behavior, that shouldn't need retesting here.
> 
> You should add some tests though that do some SystemTemplate-specific
> manipulations and test that that works out.
> 
> > diff --git a/server/tests/cimi/collections/systems_test.rb
> b/server/tests/cimi/collections/systems_test.rb
> > new file mode 100644
> > index 0000000..8496a99
> > --- /dev/null
> > +++ b/server/tests/cimi/collections/systems_test.rb
> 
> Same comment as previous test. Again, it would be good to add some tests
> that test System-specific things, even if it's only the presence of
> attributes that only Systems have.
> 
> David
> 
> 

Mime
View raw message