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:lut...@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, di...@fast.au.fujitsu.com wrote:
> > From: Dies Koper <di...@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
>
>
