On Fri, 2011-03-04 at 12:59 +0100, [email protected] wrote:
> From: Michal Fojtik <[email protected]>
>
> ---
> .../lib/deltacloud/helpers/application_helper.rb | 10 ++++-
> server/server.rb | 40 ++++++++++++++++---
> server/tests/drivers/mock/instances_test.rb | 6 +--
> 3 files changed, 42 insertions(+), 14 deletions(-)
ACK. A few comments:
> diff --git a/server/lib/deltacloud/helpers/application_helper.rb
> b/server/lib/deltacloud/helpers/application_helper.rb
> index 191a0c9..e7725dd 100644
> --- a/server/lib/deltacloud/helpers/application_helper.rb
> +++ b/server/lib/deltacloud/helpers/application_helper.rb
> @@ -126,7 +126,13 @@ module ApplicationHelper
>
> @instance = driver.send(:"#{name}_instance", credentials, params["id"])
>
> - return redirect(instances_url) if name.eql?(:destroy) or
> @instance.class!=Instance
> + if name.eql?(:destroy) or @instance.class!=Instance
It was there in the original code - any idea what the '@instance.class !
= Instance' is all about ?
Also, there's no reason to write 'name.eql?(:destroy)' - 'name
== :destroy' is equivalent, and more readable. But again, that was in
the original code.
> diff --git a/server/server.rb b/server/server.rb
> index fe723c5..b791e75 100644
> --- a/server/server.rb
> +++ b/server/server.rb
>
> @@ -373,7 +377,9 @@ END
> description "Destroy an instance."
> with_capability :destroy_instance
> param :id, :string, :required
> - control { instance_action(:destroy) }
> + control do
> + instance_action(:destroy)
> + end
That change isn't needed, really
> @@ -609,7 +623,11 @@ collection :keys do
> param :id, :string, :required
> control do
> driver.destroy_key(credentials, { :key_name => params[:id]})
> - redirect(keys_url)
> + respond_to do |format|
> + format.xml { return 204 }
> + format.json { return 204 }
> + format.html { return redirect(keys_volumes_url) }
Typo: keys_volumes_url instead of keys_url
> @@ -654,7 +672,11 @@ delete '/api/buckets/:bucket/:blob' do
> bucket_id = params[:bucket]
> blob_id = params[:blob]
> driver.delete_blob(credentials, bucket_id, blob_id)
> - redirect(bucket_url(bucket_id))
> + respond_to do |format|
> + format.xml { return 204 }
> + format.json { return 204 }
> + format.html { return redirect(url_for("/api/buckets/#{bucket_id}")) }
I'd stick with the helper 'bucket_url(bucket_id))'
David
