On Wed, 25 May 2011 21:28:00 -0400, Mo Morsi wrote: > > This patchset implements a new resource type > representing web resources/requests which can > be included in a puppet scipt. > > A default provider of this type is also included > using the curl interface for ruby as provided by > the 'curb' gem > > The second patch implements a few spec cases for > the new type and provider >
Mo,
Thanks for taking the time to work on this.
I was going to ask why the login & logout parameters were necessary, but
then I took a peek in the provider, and saw the session handling code.
It seems like being able to specify something like
web { 'login_service':
post => 'http://...',
parameters => { ... },
}
web { 'logout_service':
post => 'http://...',
parameters => { ... },
}
web { 'actual_request':
get => 'http://...',
require => Web['login_service'],
before => Web['logout_service'],
}
would be nice. Especially if you're looking to do more than one
'authenticated' request, so you're not logging in and out for each
request. Though I haven't really explored the behavior around
refresh only resources and requiring them, and it seems like this could
get complex quickly. I also haven't looked into how to reasonably share
the session across the resources (which sounds like a royal pain to do
securely and sanely).
On a related note to the session handling code, it looks like the
session will only be removed if you specify both login & logout, but not
if you only specify login. Seems like you always expect login & logout
to either both be specified, or neither, but there aren't any explicit
checks in the type to enforce this.
The two commits should really be squashed together since we prefer that
tests go in the same commit as the code they're testing.
The hanging indent on the commit message is a bit odd, and there's a
bunch of trailing whitespace, but that can all be cleaned up when it's
merged in.
The only things I would really like to see before this gets merged in is
some additional testing on the type & provider, and the commits squashed
together.
* It doesn't look like anything in the tests check that something like
'blargle' won't be accepted for the returns parameter on the type.
* put and delete don't seem to be covered by the tests for the type or
provider.
* Now that I look more closely at the provider specs, I noticed that
you seem to be stubbing to set the properties on the resource, which
you don't need to do with a real resource. You should be able to
set them directly.
--
Jacob Helwig
signature.asc
Description: Digital signature
