You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by ma...@redhat.com on 2011/07/29 00:45:28 UTC
Fix response codes for attach/detach storage volumes
The attach/detach operations for storage volumes were redirecting to the storage_volume collection. I think 202 Accepted is a better response - state of the volume is still uncertain ('unkown' reported) hence the 202?
marios
Re: [PATCH] Better response for the storage_volume attach and detach operations
Posted by Michal Fojtik <mf...@redhat.com>.
On Jul 29, 2011, at 12:45 AM, marios@redhat.com wrote:
ACK.
-- Michal
> From: marios <ma...@redhat.com>
>
>
> Signed-off-by: marios <ma...@redhat.com>
> ---
> server/server.rb | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/server/server.rb b/server/server.rb
> index f1a03f0..f4232fc 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -623,8 +623,15 @@ collection :storage_volumes do
> param :instance_id,:string, :required
> param :device, :string, :required
> control do
> - driver.attach_storage_volume(credentials, params)
> - redirect(storage_volume_url(params[:id]))
> + @storage_volume = driver.attach_storage_volume(credentials, params)
> + respond_to do |format|
> + format.html{ redirect(storage_volume_url(params[:id]))}
> + format.xml do
> + response.status = 202
> + haml :"storage_volumes/show"
> + end
> + format.json {convert_to_json(:storage_volume, @storage_volume)}
> + end
> end
> end
>
> @@ -634,9 +641,15 @@ collection :storage_volumes do
> param :id, :string, :required
> control do
> volume = driver.storage_volume(credentials, :id => params[:id])
> - driver.detach_storage_volume(credentials, :id => volume.id, :instance_id => volume.instance_id,
> - :device => volume.device)
> - redirect(storage_volume_url(params[:id]))
> + @storage_volume = driver.detach_storage_volume(credentials, :id => volume.id, :instance_id => volume.instance_id, :device => volume.device)
> + respond_to do |format|
> + format.html{ redirect(storage_volume_url(params[:id]))}
> + format.xml do
> + response.status = 202
> + haml :"storage_volumes/show"
> + end
> + format.json {convert_to_json(:storage_volume, @storage_volume)}
> + end
> end
> end
>
> --
> 1.7.3.4
>
----------------------------------------------------------------------
Michal Fojtik, Senior Software Engineer, Red Hat Czech
mfojtik@redhat.com
Deltacloud API: http://deltacloud.org
Re: [PATCH] Better response for the storage_volume attach and
detach operations
Posted by David Lutterkort <lu...@redhat.com>.
On Thu, 2011-07-28 at 16:20 -0700, David Lutterkort wrote:
> The 202 status should also be returned by JSON; I think we should just
> do something like
Actually, I just noticed that there are lots of places where we only set
the status code for XML, but not for JSON (and in some cases, also only
the location header for XML)
I'll work on a patch to clean that up tomorrow.
David
Re: [PATCH] Better response for the storage_volume attach and
detach operations
Posted by David Lutterkort <lu...@redhat.com>.
On Fri, 2011-07-29 at 01:45 +0300, marios@redhat.com wrote:
> From: marios <ma...@redhat.com>
>
>
> Signed-off-by: marios <ma...@redhat.com>
ACK, and agreed that 202 is the right status here.
> server/server.rb | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/server/server.rb b/server/server.rb
> index f1a03f0..f4232fc 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -623,8 +623,15 @@ collection :storage_volumes do
> param :instance_id,:string, :required
> param :device, :string, :required
> control do
> - driver.attach_storage_volume(credentials, params)
> - redirect(storage_volume_url(params[:id]))
> + @storage_volume = driver.attach_storage_volume(credentials, params)
> + respond_to do |format|
> + format.html{ redirect(storage_volume_url(params[:id]))}
> + format.xml do
> + response.status = 202
> + haml :"storage_volumes/show"
> + end
> + format.json {convert_to_json(:storage_volume, @storage_volume)}
> + end
The 202 status should also be returned by JSON; I think we should just
do something like
@storage_volume = driver.attach_storage_volume(credentials,
params)
status 202
respond_to do |format|
format.html { redirect(storage_volume_url(params[:id]))}
format.xml { haml :"storage_volumes/show" }
format.json { convert_to_json(:storage_volume,
@storage_volume) }
end
IOW, set the status that's right for JSON/XML before respond_to; in my
little bit of testing, HTML still gets the redirect out. But it also
makes it clearer what the actual status code for the operation is.
David
[PATCH] Better response for the storage_volume attach and detach operations
Posted by ma...@redhat.com.
From: marios <ma...@redhat.com>
Signed-off-by: marios <ma...@redhat.com>
---
server/server.rb | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/server/server.rb b/server/server.rb
index f1a03f0..f4232fc 100644
--- a/server/server.rb
+++ b/server/server.rb
@@ -623,8 +623,15 @@ collection :storage_volumes do
param :instance_id,:string, :required
param :device, :string, :required
control do
- driver.attach_storage_volume(credentials, params)
- redirect(storage_volume_url(params[:id]))
+ @storage_volume = driver.attach_storage_volume(credentials, params)
+ respond_to do |format|
+ format.html{ redirect(storage_volume_url(params[:id]))}
+ format.xml do
+ response.status = 202
+ haml :"storage_volumes/show"
+ end
+ format.json {convert_to_json(:storage_volume, @storage_volume)}
+ end
end
end
@@ -634,9 +641,15 @@ collection :storage_volumes do
param :id, :string, :required
control do
volume = driver.storage_volume(credentials, :id => params[:id])
- driver.detach_storage_volume(credentials, :id => volume.id, :instance_id => volume.instance_id,
- :device => volume.device)
- redirect(storage_volume_url(params[:id]))
+ @storage_volume = driver.detach_storage_volume(credentials, :id => volume.id, :instance_id => volume.instance_id, :device => volume.device)
+ respond_to do |format|
+ format.html{ redirect(storage_volume_url(params[:id]))}
+ format.xml do
+ response.status = 202
+ haml :"storage_volumes/show"
+ end
+ format.json {convert_to_json(:storage_volume, @storage_volume)}
+ end
end
end
--
1.7.3.4