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

Revision history for this message
John A Meinel (jameinel) wrote :

Overall LGTM.

https://codereview.appspot.com/7200049/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/7200049/diff/1/http/client.go#newcode195
http/client.go:195: for i := 0; i <= c.maxRetries; i++ {
<= ? Doesn't that mean we will try 4 times instead of 3?

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go
File identity/userpass.go (right):

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go#newcode109
identity/userpass.go:109: }
I think I have a more complete fix for this:
https://code.launchpad.net/~jameinel/goose/handle-missing-endpoint/+merge/145098

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

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.
Maybe: RFC says that Retry-After should be an int, but we don't want to
wait an entire second during the test suite.

https://codereview.appspot.com/7200049/

« Back to merge proposal