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 2/2] Added Rack::MatrixParams to support matrix params for actions
Date Sun, 06 Mar 2011 21:20:34 GMT
On Mar 5, 2011, at 1:30 AM, David Lutterkort wrote:

> 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.

Thanks, nice catch. Will keep this in mind before hitting copy&paste again ;-)

>> +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.

Cool, I didn't realized that they could be used in this way. Making it work 
shouldn't be hard to do I just remove 'if'. I'll rework the parsing part as well.

> 
>> 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.

Sure I'll split this patch set.

 -- Michal

Michal Fojtik
Software Engineer, Deltacloud API project
http://www.deltacloud.org
mfojtik@redhat.com



Mime
View raw message