You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by mf...@redhat.com on 2011/03/04 12:59:02 UTC
[PATCH core] Added 204 code after successful DELETE action
From: Michal Fojtik <mf...@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(-)
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
+ respond_to do |format|
+ format.xml { return 204 }
+ format.json { return 204 }
+ format.html { return redirect(instances_url) }
+ end
+ end
respond_to do |format|
format.xml { haml :"instances/show" }
@@ -146,7 +152,7 @@ module ApplicationHelper
def link_to_action(action, url, method)
capture_haml do
- haml_tag :form, :method => :post, :action => url, :class => :link do
+ haml_tag :form, :method => :post, :action => url, :class => [:link, method] do
haml_tag :input, :type => :hidden, :name => '_method', :value => method
haml_tag :button, :type => :submit do
haml_concat action
diff --git a/server/server.rb b/server/server.rb
index fe723c5..b791e75 100644
--- a/server/server.rb
+++ b/server/server.rb
@@ -296,7 +296,11 @@ collection :load_balancers do
param :id, :string, :required
control do
driver.destroy_load_balancer(credentials, params[:id])
- redirect(load_balancers_url)
+ respond_to do |format|
+ format.xml { return 204 }
+ format.json { return 204 }
+ format.html { return redirect(load_balancers_url) }
+ end
end
end
@@ -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
end
operation :run, :method => :post, :member => true do
@@ -479,7 +485,11 @@ collection :storage_snapshots do
param :id, :string, :required
control do
driver.create_storage_snapshot(credentials, params)
- redirect(storage_snapshot_url(params[:id]))
+ respond_to do |format|
+ format.xml { return 204 }
+ format.json { return 204 }
+ format.html { return redirect(storage_snapshots_url) }
+ end
end
end
@@ -560,7 +570,11 @@ collection :storage_volumes do
param :id, :string, :optional
control do
driver.destroy_storage_volume(credentials, params)
- redirect(storage_volumes_url)
+ respond_to do |format|
+ format.xml { return 204 }
+ format.json { return 204 }
+ format.html { return redirect(storage_volumes_url) }
+ end
end
end
@@ -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) }
+ end
end
end
@@ -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}")) }
+ end
end
#get blob metadata
@@ -763,7 +785,11 @@ collection :buckets do
param :id, :string, :required
control do
driver.delete_bucket(credentials, params[:id], params)
- redirect(buckets_url)
+ respond_to do |format|
+ format.xml { return 204 }
+ format.json { return 204 }
+ format.html { return redirect(buckets_url) }
+ end
end
end
diff --git a/server/tests/drivers/mock/instances_test.rb b/server/tests/drivers/mock/instances_test.rb
index afd42eb..14541ae 100644
--- a/server/tests/drivers/mock/instances_test.rb
+++ b/server/tests/drivers/mock/instances_test.rb
@@ -166,11 +166,7 @@ module DeltacloudUnitTest
destroy_url = (last_xml_response/'actions/link[@rel="destroy"]').first['href']
destroy_url.should_not == nil
delete create_url(destroy_url), {}, authenticate
- last_response.status.should == 302
- do_xml_request last_response.headers['Location'], {}, true
- (last_xml_response/'instances').should_not == nil
- do_xml_request "/api/instances/#{instance_id}", {}, true
- last_response.status.should == 404
+ last_response.status.should == 204
end
end
--
1.7.4
Re: [PATCH core] Added 204 code after successful DELETE action
Posted by David Lutterkort <lu...@redhat.com>.
On Sun, 2011-03-06 at 22:09 +0100, Michal Fojtik wrote:
> On Mar 5, 2011, at 1:40 AM, David Lutterkort wrote:
>
> > On Fri, 2011-03-04 at 12:59 +0100, mfojtik@redhat.com wrote:
> >> From: Michal Fojtik <mf...@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 ?
>
> No idea after half year ;-) But git blame says:
>
> 6ce87b83 server/lib/deltacloud/helpers/application_helper.rb (lutter 2010-07-08 23:48:39 +0000 129) return redirect(instances_url) if name.eql?(:destroy) or @instance.class!=Instance
>
> :-))
Hah .. that should teach me to ask before running git blame
David
Re: [PATCH core] Added 204 code after successful DELETE action
Posted by Michal Fojtik <mf...@redhat.com>.
On Mar 5, 2011, at 1:40 AM, David Lutterkort wrote:
> On Fri, 2011-03-04 at 12:59 +0100, mfojtik@redhat.com wrote:
>> From: Michal Fojtik <mf...@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 ?
No idea after half year ;-) But git blame says:
6ce87b83 server/lib/deltacloud/helpers/application_helper.rb (lutter 2010-07-08 23:48:39 +0000 129) return redirect(instances_url) if name.eql?(:destroy) or @instance.class!=Instance
:-))
Maybe we just used this helper for other collections...
> 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.
Yes, I'll try to force myself to not practice my Java habits in Ruby.
>
>> 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
Thanks, nice catch!
-- Michal
Michal Fojtik
Software Engineer, Deltacloud API project
http://www.deltacloud.org
mfojtik@redhat.com
Re: [PATCH core] Added 204 code after successful DELETE action
Posted by David Lutterkort <lu...@redhat.com>.
On Fri, 2011-03-04 at 12:59 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mf...@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