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
