[Ovirt-devel] [PATCH 2/5] API for storage pools

[Scott Seago]

David Lutterkort wrote:
> - List and filter storage pools by ipaddr, path, target and hardware_pool_id
> - Show an individual storage pool
> - Create a new storage pool
> - Destroy a storage pool
> ---
>   
I'll mention the obvious comment that we've got to filter all this stuff 
by permissions (applies to all patches). A few additional comments inline:

>  .../controllers/rest/storage_pools_controller.rb   |   60 ++++++++++++++++++++
>  wui/src/config/routes.rb                           |    1 +
>  2 files changed, 61 insertions(+), 0 deletions(-)
>  create mode 100644 wui/src/app/controllers/rest/storage_pools_controller.rb
>
> diff --git a/wui/src/app/controllers/rest/storage_pools_controller.rb b/wui/src/app/controllers/rest/storage_pools_controller.rb
> new file mode 100644
> index 0000000..99eb158
> --- /dev/null
> +++ b/wui/src/app/controllers/rest/storage_pools_controller.rb
> @@ -0,0 +1,60 @@
> +class Rest::StoragePoolsController < ApplicationController
> +
> +    EQ_ATTRIBUTES = [ :ip_addr, :export_path, :target,
> +                      :hardware_pool_id ]
> +
> +    def index
> +        conditions = []
> +        EQ_ATTRIBUTES.each do |attr|
> +            if params[attr]
> +                conditions << "#{attr} = :#{attr}"
> +            end
> +        end
> +
> +        @storage = StoragePool.find(:all,
> +                     :conditions => [conditions.join(" and "), params],
> +                     :order => "id")
> +
> +        respond_to do |format|
> +            format.xml { render :xml => @storage.to_xml }
> +        end
> +    end
> +
> +    def show
> +        @storage = StoragePool.find(params[:id])
> +        respond_to do |format|
> +            format.xml { render :xml => @storage.to_xml }
> +        end
> +    end
> +
> +    def create
> +        # Somehow the attribute 'type' never makes it through
> +        # Maybe because there is a (deprecated) Object.type ?
>   
Yes, ActiveRecord has docs that address this. You have to say foo[:type] 
rather than foo.type !
We've run into the same problem with target. foo[:target] rather than 
foo.target

I'm not sure how this affects the API though -- other than making sure 
we set/retrieve type and  target attributes correctly.
> +        pool = params[:storage_pool]
> +        type = pool[:storage_type]
> +        pool.delete(:storage_type)
> +        @storage_pool = StoragePool.factory(type, pool)
> +        respond_to do |format|
> +            if @storage_pool
> +                if @storage_pool.save
>   
We've tried to be consistent in using save! and dealing with errors in 
rescue blocks, as it's too easy to let error returns fall through this 
way. In addition, for multi-object transactions, this lets us deal with 
errors in one place for  the whole transaction.
> +                    format.xml  { render :xml => @storage_pool,
> +                        :status => :created,
> +                        :location => rest_storage_pool_url(@storage_pool) }
> +                else
> +                    format.xml  { render :xml => @storage_pool.errors,
> +                        :status => :unprocessable_entity }
>   
When we intergrate with the WUI controllers, we should standardize our 
status/message processing between the REST calls and the AJAX/json calls.
> +                end
> +            else
> +                format.xml  { render :xml => "Illegal storage type #{params[:storage_type]}", :status => :unprocessable_entity }
> +            end
> +        end
> +    end
> +
> +    def destroy
> +        @storage_pool = StoragePool.find(params[:id])
> +        @storage_pool.destroy
> +        respond_to do |format|
> +            format.xml  { head :ok }
> +        end
> +    end
> +end
>   
Scott