incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH core] Added 204 code after successful DELETE action
Date Sun, 06 Mar 2011 21:09:29 GMT
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 <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 ?

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



Mime
View raw message