Mailing-List: contact deltacloud-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltacloud-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of mfojtik@redhat.com designates
 209.132.183.28 as permitted sender)
Subject: Re: [PATCH core 2/2] Added Rack::MatrixParams to support matrix
 params for actions
Mime-Version: 1.0 (Apple Message framework v1082)
Content-Type: text/plain; charset=us-ascii
From: Michal Fojtik <mfojtik@redhat.com>
In-Reply-To: <1299285013.29138.32.camel@avon.watzmann.net>
Date: Sun, 6 Mar 2011 22:20:34 +0100
Content-Transfer-Encoding: quoted-printable
Message-Id: <86FF0691-B2CF-4146-837E-01BD5345D02A@redhat.com>
References: <1299240943-31460-1-git-send-email-mfojtik@redhat.com>
 <1299240943-31460-2-git-send-email-mfojtik@redhat.com>
 <1299285013.29138.32.camel@avon.watzmann.net>
To: deltacloud-dev@incubator.apache.org

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>
>>=20
>> ---
>> 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
>=20
> ACK with a few comments:
>=20
>> 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.
>=20
> (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=3Di-123123;test=3D=
1
>> +    #=20
>> +    # =3D> {"instance_id"=3D>"i-123123", "haha"=3D>"1", "test"=3D>"1"}=

>> +    #
>> +    # (where 'haha' is regular POST param sent from HTML form
>=20
> There is no special connection between POST and matrix params - they =
can
> be used for any request method.
>=20
> There's an added wrinkle, in that they can be used with any path
> component of a URL, as in=20
>=20
>        =
http://example.com/library;section=3Dnw/books;topic=3Dmoney;binding=3Dhard=
cover
>=20
> 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)
>=20
> 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=20
shouldn't be hard to do I just remove 'if'. I'll rework the parsing part =
as well.

>=20
>> 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 =3D> instance_action, :method =3D> =
instance_action_method(instance_action), :href =3D> =
self.send("#{instance_action}_instance_url", @instance.id)}
>>       - if driver.respond_to?(:run_on_instance)
>> -        %link{:rel =3D> 'run', :method =3D> :post, :href =3D> =
run_instance_url(@instance.id)}
>> +        %link{:rel =3D> 'run', :method =3D> :post, :href =3D> =
"#{run_instance_url(@instance.id)};id=3D#{@instance.id}"}
>>           - generate_action_params(:instances, :run) do |param|
>>             =3D param.call(:id =3D> @instance.id, :password =3D> =
@instance.password)
>>       - if @instance.can_create_image?
>> -        %link{:rel =3D> 'create_image', :method =3D> :post, :href =3D>=
 create_image_url}
>> +        %link{:rel =3D> 'create_image', :method =3D> :post, :href =3D>=
 "#{create_image_url};instance_id=3D#{@instance.id}"}
>>           - generate_action_params(:images, :create) do |param|
>>             =3D param.call(:instance_id =3D> @instance.id)
>>   - if @instance.instance_variables.include?("@launch_time")
>=20
> 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