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 2/2] Added Rack::MatrixParams to support matrix params for actions
Date Sat, 05 Mar 2011 00:30:13 GMT
On Fri, 2011-03-04 at 13:15 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> ---
>  server/lib/sinatra/rack_matrix_params.rb |   54 ++++++++++++++++++++++++++++++
>  server/server.rb                         |    2 +
>  server/views/instances/show.xml.haml     |    4 +-
>  3 files changed, 58 insertions(+), 2 deletions(-)
>  create mode 100644 server/lib/sinatra/rack_matrix_params.rb

ACK with a few comments:

> diff --git a/server/lib/sinatra/rack_matrix_params.rb b/server/lib/sinatra/rack_matrix_params.rb
> new file mode 100644
> index 0000000..7488c73
> --- /dev/null
> +++ b/server/lib/sinatra/rack_matrix_params.rb
> @@ -0,0 +1,54 @@
> +# Copyright (C) 2009, 2010  Red Hat, Inc.

(C) 2011 since you just wrote this.

> +module Rack
> +
> +    # This will allow to use 'matrix' params in POST requests, like:
> +    #
> +    # POST http://127.0.0.1:9393/test/test;person_id=i-123123;test=1
> +    # 
> +    # => {"instance_id"=>"i-123123", "haha"=>"1", "test"=>"1"}
> +    #
> +    # (where 'haha' is regular POST param sent from HTML form

There is no special connection between POST and matrix params - they can
be used for any request method.

There's an added wrinkle, in that they can be used with any path
component of a URL, as in 

        http://example.com/library;section=nw/books;topic=money;binding=hardcover

You should really parse the URI, take the path, split it on '/', and
split each of the resulting components on ';' (for now, it's enough to
do that for the last path component)

There's of course the danger that different path components have matrix
params with the same name, but I'd just ignore that for now.

> diff --git a/server/views/instances/show.xml.haml b/server/views/instances/show.xml.haml
> index 5e1b974..7e9f4fa 100644
> --- a/server/views/instances/show.xml.haml
> +++ b/server/views/instances/show.xml.haml
> @@ -22,11 +22,11 @@
>        - @instance.actions.compact.each do |instance_action|
>          %link{:rel => instance_action, :method => instance_action_method(instance_action),
:href => self.send("#{instance_action}_instance_url", @instance.id)}
>        - if driver.respond_to?(:run_on_instance)
> -        %link{:rel => 'run', :method => :post, :href => run_instance_url(@instance.id)}
> +        %link{:rel => 'run', :method => :post, :href => "#{run_instance_url(@instance.id)};id=#{@instance.id}"}
>            - generate_action_params(:instances, :run) do |param|
>              = param.call(:id => @instance.id, :password => @instance.password)
>        - if @instance.can_create_image?
> -        %link{:rel => 'create_image', :method => :post, :href => create_image_url}
> +        %link{:rel => 'create_image', :method => :post, :href => "#{create_image_url};instance_id=#{@instance.id}"}
>            - generate_action_params(:images, :create) do |param|
>              = param.call(:instance_id => @instance.id)
>    - if @instance.instance_variables.include?("@launch_time")

I'd much prefer if this patch series was such that patch 1/2 added the
new rack middleware (including using it in server.rb), and patch 2/2 did
the create_image stuff.

David



Mime
View raw message