On Fri, 2013-02-08 at 22:44 +1100, di...@fast.au.fujitsu.com wrote:
> From: Dies Koper <di...@fast.au.fujitsu.com>
You killed yourself with debug prints; I'll point the exact place out
below. But I also have a couple more comments.
> diff --git a/server/lib/cimi/collections/addresses.rb
> b/server/lib/cimi/collections/addresses.rb
> index 9ea1764..a51e658 100644
> --- a/server/lib/cimi/collections/addresses.rb
> +++ b/server/lib/cimi/collections/addresses.rb
> @@ -20,7 +20,7 @@ module CIMI::Collections
>
> collection :addresses do
>
> - description 'An Address represents an IP address, and its associated
> metdata, for a particular Network.'
> + description 'An Address represents an IP address, and its associated
> metadata, for a particular Network.'
>
> operation :index, :with_capability => :addresses do
> description 'List all Addresses in the AddressCollection'
> diff --git a/server/lib/cimi/collections/machines.rb
> b/server/lib/cimi/collections/machines.rb
> index 5718f46..9a71bb1 100644
> --- a/server/lib/cimi/collections/machines.rb
> +++ b/server/lib/cimi/collections/machines.rb
> @@ -85,7 +85,7 @@ module CIMI::Collections
> end
>
> action :restart, :with_capability => :reboot_instance do
> - description "Start specific machine."
> + description "Restart specific machine."
> param :id, :string, :required
> control do
> machine = Machine.find(params[:id], self)
These fixes should go into separate patches (whether one or two is up to
you) But there's no shame in sending one line patches - it's actually
nice because they are much easier to review. It's very important that
each patch is as small as possible for that reason. 'Small' here means
the smallest change that will allow the tests still to pass.
> 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
Nothing wrong with what you did here, but I am wondering if there is
anything we can do to reduce this sort of boilerplate. Most of our
collections look exactly the same ...
> diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> index c74b8c3..2c49c75 100644
> --- a/server/lib/cimi/models/base.rb
> +++ b/server/lib/cimi/models/base.rb
> @@ -63,7 +63,7 @@ require_relative '../helpers/database_helper'
> # [struct(name, opts, &block)]
> # A structured subobject; the block defines the schema of the
> # subobject. The +:content+ option can be used to specify the attribute
> -# that should receive the content of hte corresponding XML element
> +# that should receive the content of the corresponding XML element
> # [array(name, opts, &block)]
> # An array of structured subobjects; the block defines the schema of
> # the subobjects.
> @@ -86,7 +86,7 @@ module CIMI::Model
> #
> # Common attributes for all resources
> #
> - text :id, :name, :description, :created
> + text :id, :name, :description, :created, :updated
Another change that could go into a separate patch - you are right that
we are missing these from the common attributes (section 5.10.1 in the
spec)
> diff --git a/server/lib/cimi/models/system.rb
> b/server/lib/cimi/models/system.rb
> new file mode 100644
> index 0000000..619e7a9
> --- /dev/null
> +++ b/server/lib/cimi/models/system.rb
...
> + def self.find(id, context)
> + puts "system#self.find called"
> + systems = []
> + if id == :all
> + systems = context.driver.systems(context.credentials, {:env=>context})
> +puts "systems from driver #{systems.size}"
> + else
> + system = context.driver.system(context.credentials, {:env=>context,
> :id=>id})
> + raise CIMI::Model::NotFound unless system
> + system
> + end
> + end
The puts just before the else is where things go wrong: without the
puts, the value of the if branch is whatever is assigned to systems,
which in turn is what gets returned from self.find - by inserting the
puts, you are now returning nil. Put a line 'systems' just after the
puts, and things look much better (at least, you're getting a valid
response back instead of a 500, though it had no systems for me)
As a general comment, try adding as little of the System* objects in one
patch as possible, again, to make reviewing easier. I haven't looked in
detail if that makes sense here or not, but it would be nice if it did,
and there's nothing wrong with a CIMI provider that only has System, but
none of the other System* collections.
David
