Code review comment for lp://staging/~wallyworld/goose/rate-limit-retry-tests

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
> https://code.launchpad.net/%7Ejameinel/goose/handle-missing-endpoint/+merge/145098
>
> I had a quick look, the mp was missing the pre-req branch so was
> very large. Looked ok at first glance. Perhaps we can land this
> branch now as is and follow up with yours?

Yeah, goose-bot's trunk wasn't up to date with lp:goose, and which I
based my patch on. I tried to re-push so this should be cleaned up.

As for your patch, I think it is an accurate fix, but my concern is
just a more general thing. When we encounter edge cases the code
wasn't covering (a service that is only exported in one region), we
really need a test case for it, because those are the sorts of things
we are going to forget about in 6 months when we go tweaking things
for HP vs Canonistack vs Rackspace, etc.

>
> https://codereview.appspot.com/7200049/diff/1/nova/live_test.go
> File nova/live_test.go (right):
>
> https://codereview.appspot.com/7200049/diff/1/nova/live_test.go#newcode452
>
>
nova/live_test.go:452: return
> On 2013/01/28 06:44:57, jameinel wrote:
>> Is it possible to use your hooks to check for a retry request,
>> rather
> than
>> reading the output string? (I realize this probably didn't exist
> before, but it
>> seems like a really good use of it.)
>
> Possibly, but the hook stuff has been set up to work with the test
> doubles (manipulates the state of the test service back end) rather
> than a real live instance. The nova client implementation would
> need to be retooled to implement the ServiceControl interface.
> Perhaps this can be done in a followup branch.

I guess it is hooking the client-side stuff, vs hooking the service
side stuff.

>
> https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go
>
>
File testservices/novaservice/service_http.go (right):
>
> https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go#newcode216
>
>
testservices/novaservice/service_http.go:216: // Retry-After is usually
> an int but we don't want the tests taking too long to run. On
> 2013/01/28 06:44:57, jameinel wrote:
>> Maybe: RFC says that Retry-After should be an int, but we don't
>> want
> to wait an
>> entire second during the test suite.
>
> Done.
>
> https://codereview.appspot.com/7200049/
>

LGTM.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEHUQ0ACgkQJdeBCYSNAAMvGQCfRQtVLUas22F84lPzovlpzzZK
p0IAoItwzHDxMx7CSrVW7Q6X7sYFrNfA
=dice
-----END PGP SIGNATURE-----