Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(27)

Issue 6924043: Second phase of nova testing service: HTTP API. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by dimitern
Modified:
6 years, 7 months ago
Reviewers:
mp+139089, rog
Visibility:
Public.

Description

Second phase of nova testing service: HTTP API. Implemented common HTTP API handling as well as flavors API. This is the first step towards the rest of the HTTP API implementation. https://code.launchpad.net/~dimitern/goose/nova-testing-service-http-flavors/+merge/139089 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 50

Patch Set 2 : Second phase of nova testing service: HTTP aPI. #

Total comments: 52

Patch Set 3 : Second phase of nova testing service: HTTP API. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+642 lines, -33 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M testservices/novaservice/service.go View 1 2 4 chunks +25 lines, -13 lines 0 comments Download
M testservices/novaservice/service_http.go View 1 2 1 chunk +315 lines, -4 lines 0 comments Download
M testservices/novaservice/service_http_test.go View 1 2 3 chunks +288 lines, -2 lines 3 comments Download
M testservices/novaservice/service_test.go View 1 2 3 chunks +12 lines, -14 lines 0 comments Download

Messages

Total messages: 10
dimitern
Please take a look.
6 years, 7 months ago (2012-12-10 20:46:58 UTC) #1
TheMue
Not yet got the full logic but so far some first remarks. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/service.go File testservices/novaservice/service.go ...
6 years, 7 months ago (2012-12-11 08:45:01 UTC) #2
rog
generally looks good. here's a first round of comments - i've only had a brief ...
6 years, 7 months ago (2012-12-11 10:31:42 UTC) #3
jameinel
Overall I like where this is at. I haven't reviewed everything, but I figure some ...
6 years, 7 months ago (2012-12-11 11:20:20 UTC) #4
dimitern
Please take a look. https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/service.go File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/1/testservices/novaservice/service.go#newcode47 testservices/novaservice/service.go:47: ep := strings.TrimRight(n.hostname, "/") + ...
6 years, 7 months ago (2012-12-12 11:00:47 UTC) #5
rog
LGTM with some minor comments below. https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/service.go File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/service.go#newcode28 testservices/novaservice/service.go:28: // endpoint URL ...
6 years, 7 months ago (2012-12-12 15:02:32 UTC) #6
fwereade
LGTM, but a few suggestions below https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/service.go File testservices/novaservice/service.go (right): https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/service.go#newcode32 testservices/novaservice/service.go:32: ep += baseURL ...
6 years, 7 months ago (2012-12-12 15:18:44 UTC) #7
dimitern
*** Submitted: Second phase of nova testing service: HTTP API. Implemented common HTTP API handling ...
6 years, 7 months ago (2012-12-12 17:32:24 UTC) #8
rog
a few after-the-fact comments, which you may wish to take account of in a future ...
6 years, 7 months ago (2012-12-12 18:12:13 UTC) #9
dimitern
6 years, 7 months ago (2012-12-13 11:08:20 UTC) #10
Message was sent while issue was closed.
On 2012/12/12 18:12:13, rog wrote:
> a few after-the-fact comments, which you may wish to take account of in a
future
> CL.
> 
>
https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser...
> File testservices/novaservice/service_http.go (right):
> 
>
https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser...
> testservices/novaservice/service_http.go:238: resp.Flavors = entities
> On 2012/12/12 15:18:44, fwereade wrote:
> >     resp := struct {
> >         Flavors []nova.Entity
> >     }{entities}
> > 
> > (I don't think you need to bother with the explicit json name when it's what
> > would be picked anyway (it will be picked anyway, right?))
> 
> nope. json names are marshalled as is. it's only unmarshalling that has some
> case-folding logic AFAIK
> 
>
https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser...
> File testservices/novaservice/service_http_test.go (right):
> 
>
https://codereview.appspot.com/6924043/diff/9001/testservices/novaservice/ser...
> testservices/novaservice/service_http_test.go:81: // workaround for
> https://code.google.com/p/go/issues/detail?id=4454
> On 2012/12/12 17:32:24, dimitern wrote:
> > On 2012/12/12 15:02:33, rog wrote:
> > > i thought that issue was triggered by the NoContent code only
> > 
> > Any time when len(body) == 0, hence the transfer-encoding fails to serialize
> it
> > properly (or the client fails to parse it, whatever).
> 
> i'd be surprised if that was true. can you reproduce the bug without using
> StatusNoContent?
> 
> (also, the bug was in server code, not client code, which this is)
> 
>
https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser...
> File testservices/novaservice/service_http_test.go (right):
> 
>
https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser...
> testservices/novaservice/service_http_test.go:107: func makeFlavors(flavor
> ...nova.FlavorDetail) []nova.FlavorDetail {
> you could just return flavor here (no need for the append).
> 
> alternatively, you could do:
> 
> type flavorSlice []nova.FlavorDetail
> 
> then instead of the makeFlavors call below, you'd use a more concise version:
> 
>    flavorSlice{{Id: "fl1"}}
> 
>
https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser...
> testservices/novaservice/service_http_test.go:119: var simpleTests = []struct
{
> much nicer, thanks.
> 
>
https://codereview.appspot.com/6924043/diff/9002/testservices/novaservice/ser...
> testservices/novaservice/service_http_test.go:234: c.Logf("#%d. %s %s ->
%d\n",
> i+1, t.method, t.url, t.expect.code)
> it's conventional to print the index (i) unchanged.
> we don't mind if counting starts at zero.

Thanks for the suggestions, I'll add the to the next CL along with the rest of
the HTTP API implementation.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b