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 basic IP address managment support (for EC2)
Date Mon, 02 May 2011 23:25:55 GMT
On Mon, 2011-05-02 at 13:40 +0200, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>

ACK after addressing the comments below:
 
> diff --git a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> index 4b9899a..fc9ab73 100644
> --- a/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> +++ b/server/lib/deltacloud/drivers/ec2/ec2_driver.rb
> @@ -509,6 +509,56 @@ module Deltacloud
>            end
>          end
>  
> +        def addresses(credentials, opts={})
> +          ec2 = new_client(credentials)
> +          address_id = (opts and opts[:id]) ? opts[:id] : []

Shouldn't address_id always be an array, i.e.,

        address_id = (opts and opts[:id]) ? [opts[:id]] : []

Also a request for http://localhost:3001/api/addresses?id=127.0.0.1
leads to a backend error (error code 502)

It would be more natural to me if that just resulted in an empty list.

A request for http://localhost:3001/api/addresses/127.0.0.1 also leads
to a 502; that should be a 404

> diff --git a/server/server.rb b/server/server.rb
> index dcc76e5..344d4bf 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -858,3 +858,89 @@ collection :buckets do
>    end
>  
>  end
> +
> +get '/api/addresses/:id/associate' do
> +  @instances = driver.instances(credentials)
> +  @address = Address::new(:id => params[:id])
> +  respond_to do |format|
> +    format.html { haml :"addresses/associate" }
> +  end
> +end
> +
> +collection :addresses do
> +  description "Manage IP addresses"
> +
> +  operation :index do
> +    description "List IP addresses assigned to your account."
> +    with_capability :addresses
> +    control do
> +      filter_all :addresses
> +    end
> +  end
> +
> +  operation :show do
> +    description "Show details about IP addresses specified by given ID"
> +    with_capability :address
> +    param :id,  :string,  :required
> +    control { show :address }
> +  end
> +
> +  operation :create do
> +    description "Acquire a new IP address for use with your account."
> +    with_capability :create_address
> +    control do
> +      @address = driver.create_address(credentials, {})
> +      respond_to do |format|
> +        format.html { haml :"addresses/show" }
> +        format.xml do
> +          response.status = 201  # Created

Also set the Location header to the URL for the new address.


> diff --git a/server/views/addresses/index.xml.haml b/server/views/addresses/index.xml.haml
> new file mode 100644
> index 0000000..e2a564f
> --- /dev/null
> +++ b/server/views/addresses/index.xml.haml
> @@ -0,0 +1,4 @@
> +!!!XML
> +%addresses
> +  - @elements.each do |c|
> +    = haml :'address/show', :locals => { :@address => c, :partial => true }

Typo:
                ^^^^ addresses/show

> diff --git a/server/views/addresses/show.xml.haml b/server/views/addresses/show.xml.haml
> new file mode 100644
> index 0000000..a450690
> --- /dev/null
> +++ b/server/views/addresses/show.xml.haml
> @@ -0,0 +1,12 @@
> +- unless defined?(partial)
> +  !!! XML
> +%address{ :href => address_url(@address.id), :id => @address.id }

We should provide the IP as a separate attribute; there's no need to
cement ID == IP in the API.

David


Mime
View raw message