incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core] Added 204 code after successful DELETE action
Date Sat, 05 Mar 2011 00:40:13 GMT
On Fri, 2011-03-04 at 12:59 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> ---
>  .../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


Mime
View raw message