Issue 6940073: nova double: Rest of HTTP API. (Closed)
|
Created:
12 years, 4 months ago by dimitern Modified:
12 years, 3 months ago Reviewers:
mp+140242 Visibility:
Public. |
Descriptionnova double: Rest of HTTP API.
This implements the rest of the HTTP API of the Nova testing double:
* servers
* servers/detail
* servers/<id>/os-security-groups
* servers/<id>/action (add/removeSecurityGroup; add/removeFloatingIp)
* os-security-groups
* os-security-group-rules
* os-floating-ips
There are a few things to change in order to integrate with the Nova client and the local "live" tests, but this will come in a follow-up.
https://code.launchpad.net/~dimitern/goose/nova-testing-service-http2/+merge/140242
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 58
Patch Set 2 : nova double: Rest of HTTP API. #
Total comments: 80
Patch Set 3 : nova double: Rest of HTTP API. #
Total comments: 24
Patch Set 4 : nova double: Rest of HTTP API. #
Total comments: 4
Patch Set 5 : nova double: Rest of HTTP API. #
MessagesTotal messages: 12
Please take a look.
Sign in to reply to this message.
Not a full review, mostly just typos, but a couple of comments nonetheless. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:384: // allServerSecurtyGroups returns all security groups attached to the s/urty/urity/ https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:159: // replaceVars replaces $ENDPOINT$, $URL$, $ID, and $ERROR$ in the s/ID/ID$/ https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:160: // response body with their values, taking the original requset into s/requset/request/ https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:257: func getId(prefix string, r *http.Request) string { What does this do that `path.Base(r.URL.Path)` doesn't? https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: path := strings.Replace(r.URL.Path, prefix, "", 1) OK, it does this, but I don't understand why, or what it changes. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:278: resp.Flavor = *flavor You can compress this a touch as mentioned in a previous CL https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:360: // handleServerActions handles the servers/<id/action HTTP API. s/id/id>/
Sign in to reply to this message.
This is quite a big change to follow all at once, so I'm still not sure this is a full review, but it should have enough comments to be going on with. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:567: notFoundResponse.send(w, r) Can we not just do all the getId/notFoundResponse stuff once at the top of the function? losing the unknown-request-method panic doesn't seem like much of a price to pay for the simpler code. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:579: func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) int { Is 0 really not a valid id? I think we're packing too much into a single return value here. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:641: nextId := len(n.allSecurityGroups()) + 1 I don't think this guarantees a unique id -- you need a separate counter, don't you? https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:706: nextId := len(n.rules) + 1 Similarly https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:790: nextId := len(n.allFloatingIPs()) + 1 Likewise. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:824: } A lot of these functions feel just a little bit too big for my personal comfort, and I find myself wanting something like a `handler` interface with verb-specific methods... but that may be overdoing it. Just a thought, probably not a sensible one :). https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:444: }, Nice to see all these :)
Sign in to reply to this message.
looking reasonable. i think it can be simplified a bit - some comments below. more comments to come, but this should be good to be going on with. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:384: // allServerSecurtyGroups returns all security groups attached to the s/Securty/Security/ https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:159: // replaceVars replaces $ENDPOINT$, $URL$, $ID, and $ERROR$ in the s/$ID/$ID$/ https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:257: func getId(prefix string, r *http.Request) string { On 2012/12/17 21:04:11, fwereade wrote: > What does this do that `path.Base(r.URL.Path)` doesn't? +1 i don't really understand the purpose of this function. perhaps putting a little example of the transformation it's expected to perform in the doc comment might help. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:361: func (n *Nova) handleServerActions(server nova.ServerDetail, w http.ResponseWriter, r *http.Request) { it seems to me that this function might look better if, instead of sending each response, it just returns an error. func (n *Nova) handleServerActions(server nova.ServerDetail, w http.ResponseWriter, r *http.Request) error i suggest that you could also make a new type, say errorResponse, that knows how to send itself as a response, but also implements the error interface. then notFoundResponse, noContentResponse etc become instances of errorResponse, and your code can simply return them as errors. then the caller can do: if err := n.handleServerActons(....); err != nil { sendError(err, w, r) } sendError would check if the error is of type errorResponse, and ask the error to send itself if so, otherwise it would bundle up the error into something like the current errorResponse. a similar comment applies to the other handler functions below. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:368: var action map[string]interface{} ISTM that you could make this more robust (so the server won't panic if you send it an action with missing fields) if you do something like: var action struct { AddSecurityGroup *struct { Name string } RemoveSecurityGroup *struct { Name string } AddFloatingIP *struct { Address string } RemoveFloatingIP *struct { Address string } } then you could do: switch { case action.AddSecurityGroup != nil: name := action.AddSecurityGroup.Name case action.RemoveSecurityGroup != nil: ... etc } https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:369: err = json.Unmarshal(body, &action) it's fairly common to do this on one line: if err := json.Unmarshal(body, &action); err != nil { ... } https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:466: if len(srvGroups) == 0 { why is this necessary? https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:484: if len(entities) == 0 { why is this necessary? https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:579: func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) int { i'm not that keen on the way this function might or might not send a response, and the fact that whether it has done so is encoded in the id. assuming you go with my suggestion for handleServerAction above, then it might be better like this: func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) (int, error) and have it return a particular error value if the id is not present: var idNotPresent = errors.New("group id not present") (and, as suggested above, badRequestSGResponse and notFoundJSONResponse would both be errors and could be returned as the error value) https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:628: if err != nil || len(body) == 0 { is there a particular reason for the len(body) check? ISTM that the Unmarshal will fail in this case anyway.
Sign in to reply to this message.
LGTM. Some tweaks. I think this is a case of post commit review, and having us going over the code and cleaning it up as necessary. As this is testing infrastructure, I'm ok having it slightly cruftier, as long as it is still maintainable. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:361: func (n *Nova) handleServerActions(server nova.ServerDetail, w http.ResponseWriter, r *http.Request) { I would have expected you to take a *nova.ServerDetail rather than have callers create a memory copy to pass it in. (your caller already has a *nova.ServerDetail anyway). Is there a reason to make it take the struct itself? (I'm open to advice, but certainly in C pretty much everything takes *struct not struct) https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:368: var action map[string]interface{} It seems that creating a type for this might make bits easier to read. type GenericMap map[string]interface{} Alternatively, there aren't that many values that we actually support. So doing: type actionMap struct { addSecurityGroup map[string]interface{} removeSecurityGroup map[string]interface{} ... } And then later on you can do: if action.addSecurityGroup != nil { } ... This can certainly be done as a follow up. You're welcome to disagree, but my immediate thought is that: + if ac := action["addSecurityGroup"]; ac != nil { + name := ac.(map[string]interface{})["name"].(string) is a bit hard to parse, while if we defined the expected types into a struct, you could easily read it. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:447: if serverId := getId("servers", r); serverId != "" { Maybe rename this to "maybeServerId", and then later do: if maybeServerId == "os-security-groups" { ... } else { serverId = maybeServerId } Not required, but it does hint that William's comment about getId is valid. Instead of calling it getId, call it "getTail" and do something reasonable with that. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:490: if serverId == "action" { Similarly here. getId doesn't always return an ID, so it is probably inappropriately named. Maybe we should be doing it differently, and parsing from the top, rather than from the tail. Perhaps just splitting it into parts, and doing something based on the number of parts? https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:824: } Putting these into a map does feel a little bit better than a lot of switch statements. For the first implementations, it was overkill. This one is starting to get big enough to make more sense. I wouldn't block landing this, though.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:384: // allServerSecurtyGroups returns all security groups attached to the On 2012/12/17 21:04:11, fwereade wrote: > s/urty/urity/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:384: // allServerSecurtyGroups returns all security groups attached to the On 2012/12/18 11:34:40, rog wrote: > s/Securty/Security/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:159: // replaceVars replaces $ENDPOINT$, $URL$, $ID, and $ERROR$ in the On 2012/12/17 21:04:11, fwereade wrote: > s/ID/ID$/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:159: // replaceVars replaces $ENDPOINT$, $URL$, $ID, and $ERROR$ in the On 2012/12/18 11:34:40, rog wrote: > s/$ID/$ID$/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:160: // response body with their values, taking the original requset into On 2012/12/17 21:04:11, fwereade wrote: > s/requset/request/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:257: func getId(prefix string, r *http.Request) string { On 2012/12/17 21:04:11, fwereade wrote: > What does this do that `path.Base(r.URL.Path)` doesn't? I don't know - never heard of it, will take a look, 10x! https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:257: func getId(prefix string, r *http.Request) string { On 2012/12/18 11:34:40, rog wrote: > On 2012/12/17 21:04:11, fwereade wrote: > > What does this do that `path.Base(r.URL.Path)` doesn't? > > +1 > i don't really understand the purpose of this function. > perhaps putting a little example of the transformation it's expected to perform > in the doc comment might help. I replaced it with path.Base(r.URL.Path) https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:258: path := strings.Replace(r.URL.Path, prefix, "", 1) On 2012/12/17 21:04:11, fwereade wrote: > OK, it does this, but I don't understand why, or what it changes. It's because a certain responses (actually only 2 I think) require to have a link build from the base path + current URL path. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:278: resp.Flavor = *flavor On 2012/12/17 21:04:11, fwereade wrote: > You can compress this a touch as mentioned in a previous CL You mean the json tag? No, it won't work without it. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:360: // handleServerActions handles the servers/<id/action HTTP API. On 2012/12/17 21:04:11, fwereade wrote: > s/id/id>/ Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:361: func (n *Nova) handleServerActions(server nova.ServerDetail, w http.ResponseWriter, r *http.Request) { On 2012/12/18 13:12:49, jameinel wrote: > I would have expected you to take a *nova.ServerDetail rather than have callers > create a memory copy to pass it in. (your caller already has a > *nova.ServerDetail anyway). > > Is there a reason to make it take the struct itself? (I'm open to advice, but > certainly in C pretty much everything takes *struct not struct) True, fixed. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:361: func (n *Nova) handleServerActions(server nova.ServerDetail, w http.ResponseWriter, r *http.Request) { On 2012/12/18 11:34:40, rog wrote: > it seems to me that this function might look better if, instead of sending each > response, it just returns an error. > > func (n *Nova) handleServerActions(server nova.ServerDetail, w > http.ResponseWriter, r *http.Request) error > > i suggest that you could also make a new type, say errorResponse, that knows how > to send itself as a response, but also implements the error interface. then > notFoundResponse, noContentResponse etc become instances of errorResponse, and > your code can simply return them as errors. > > > then the caller can do: > > if err := n.handleServerActons(....); err != nil { > sendError(err, w, r) > } > > sendError would check if the error is of type errorResponse, and ask the error > to send itself if so, otherwise it would bundle up the error into something like > the current errorResponse. > > a similar comment applies to the other handler functions below. Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:368: var action map[string]interface{} On 2012/12/18 11:34:40, rog wrote: > ISTM that you could make this more robust (so the server won't panic if you send > it an action with missing fields) if you do something like: > > var action struct { > AddSecurityGroup *struct { > Name string > } > RemoveSecurityGroup *struct { > Name string > } > AddFloatingIP *struct { > Address string > } > RemoveFloatingIP *struct { > Address string > } > } > > then you could do: > > switch { > case action.AddSecurityGroup != nil: > name := action.AddSecurityGroup.Name > case action.RemoveSecurityGroup != nil: > ... etc > } Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:368: var action map[string]interface{} On 2012/12/18 13:12:49, jameinel wrote: > It seems that creating a type for this might make bits easier to read. > > type GenericMap map[string]interface{} > > Alternatively, there aren't that many values that we actually support. So doing: > > type actionMap struct { > addSecurityGroup map[string]interface{} > removeSecurityGroup map[string]interface{} > ... > } > > And then later on you can do: > > if action.addSecurityGroup != nil { > } > ... > > This can certainly be done as a follow up. You're welcome to disagree, but my > immediate thought is that: > > + if ac := action["addSecurityGroup"]; ac != nil { > + name := ac.(map[string]interface{})["name"].(string) > > is a bit hard to parse, while if we defined the expected types into a struct, > you could easily read it. Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:369: err = json.Unmarshal(body, &action) On 2012/12/18 11:34:40, rog wrote: > it's fairly common to do this on one line: > > if err := json.Unmarshal(body, &action); err != nil { > ... > } Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:447: if serverId := getId("servers", r); serverId != "" { On 2012/12/18 13:12:49, jameinel wrote: > Maybe rename this to "maybeServerId", and then later do: > > if maybeServerId == "os-security-groups" { ... > } else { > serverId = maybeServerId > } > > Not required, but it does hint that William's comment about getId is valid. > Instead of calling it getId, call it "getTail" and do something reasonable with > that. I removed getId altogether and also changed this. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:466: if len(srvGroups) == 0 { On 2012/12/18 11:34:40, rog wrote: > why is this necessary? I think so, because otherwise I get {"security_groups":null} in the response, rather than []. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:484: if len(entities) == 0 { On 2012/12/18 11:34:40, rog wrote: > why is this necessary? See previous. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:490: if serverId == "action" { On 2012/12/18 13:12:49, jameinel wrote: > Similarly here. getId doesn't always return an ID, so it is probably > inappropriately named. > > Maybe we should be doing it differently, and parsing from the top, rather than > from the tail. Perhaps just splitting it into parts, and doing something based > on the number of parts? Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:567: notFoundResponse.send(w, r) On 2012/12/18 10:36:47, fwereade wrote: > Can we not just do all the getId/notFoundResponse stuff once at the top of the > function? losing the unknown-request-method panic doesn't seem like much of a > price to pay for the simpler code. OK, I'll try this. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:579: func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) int { On 2012/12/18 10:36:47, fwereade wrote: > Is 0 really not a valid id? I think we're packing too much into a single return > value here. Well, AFAIK 0 is not a valid ID. Anyway, I changed the logic here. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:579: func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) int { On 2012/12/18 11:34:40, rog wrote: > i'm not that keen on the way this function might or might not send a response, > and the fact that whether it has done so is encoded in the id. > > assuming you go with my suggestion for handleServerAction above, then it might > be better like this: > > func (n *Nova) processGroupId(w http.ResponseWriter, r *http.Request) (int, > error) > > and have it return a particular error value if the id is not present: > > var idNotPresent = errors.New("group id not present") > > (and, as suggested above, badRequestSGResponse and notFoundJSONResponse would > both be errors and could be returned as the error value) > I changed it so now it's better I think. Not returning an error, because this will complicate the logic, but take a look. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:628: if err != nil || len(body) == 0 { On 2012/12/18 11:34:40, rog wrote: > is there a particular reason for the len(body) check? ISTM that the Unmarshal > will fail in this case anyway. Yes, because this 400 is a known (observed) response, while the sendError sends 500 (faked) one with the error message. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:641: nextId := len(n.allSecurityGroups()) + 1 On 2012/12/18 10:36:47, fwereade wrote: > I don't think this guarantees a unique id -- you need a separate counter, don't > you? Yeah, I guess so, although OS seems to use something similar, since when you remove and recreate the same thing (e.g. floating IP) it has the same ID. Anyway, I can add a counter. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:706: nextId := len(n.rules) + 1 On 2012/12/18 10:36:47, fwereade wrote: > Similarly Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:790: nextId := len(n.allFloatingIPs()) + 1 On 2012/12/18 10:36:47, fwereade wrote: > Likewise. Done. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:824: } On 2012/12/18 13:12:49, jameinel wrote: > Putting these into a map does feel a little bit better than a lot of switch > statements. For the first implementations, it was overkill. This one is starting > to get big enough to make more sense. > > I wouldn't block landing this, though. I changed it as William suggested. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:824: } On 2012/12/18 10:36:47, fwereade wrote: > A lot of these functions feel just a little bit too big for my personal comfort, > and I find myself wanting something like a `handler` interface with > verb-specific methods... but that may be overdoing it. Just a thought, probably > not a sensible one :). I'll do this in a follow up CL. https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6940073/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:444: }, On 2012/12/18 10:36:47, fwereade wrote: > Nice to see all these :) Yeah, I'm getting addicted to table-based testing :)
Sign in to reply to this message.
another round of comments. it's looking nice. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:40: "", all these errors should have meaningful error text (in case someone decides to print them out, for logging or whatever). https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:129: errNoContent = &errorResponse{ we agreed online that this doesn't count as an error. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:135: errAccepted = &errorResponse{ as above. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:147: errInternal = &errorResponse{ since this isn't a single error like the others, but more a template for a class of errors, i think this would probably be better crafted when an internal error is created (see below). https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:172: if e.Error() != "" { i think this is wrong - all the err* values should have e.Error() != "". if we inline the creation of errInternal (see below) i think we can lose this logic. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:185: var body []byte perhaps: body := e.requestBody() and rename replaceVars to requestBody and put the e.body != "" test into it. slightly less code and a better division of responsibilities perhaps? https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:198: in a future CL, it may be worth moving the meta-handler code into a separate file from all the actual handlers. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:217: if err == nil { merge with previous line? https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:220: resp, _ := err.(http.Handler) these few lines have a few problems. i'd suggest something like this instead: if resp, _ := err.(http.Handler); resp != nil { resp.ServeHTTP(w, r) return } resp := &errorResp{ ... details taken from errInternal } resp.ServeHTTP(w, r) https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:222: resp := errInternal this shadows the resp above, and hence will be ignored. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:223: resp.errorText = err.Error() this changes the global instance of errInternal, which probably isn't what you want. to make it work, you should do something like: iresp := *errInternal iresp.errorText = err.Error() resp = &iresp but perhaps better just inlining the errInternal creation. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:230: func sendError(err error, w http.ResponseWriter, r *http.Request) error { i don't think we need this function any more https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:245: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:297: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:327: var resp struct { as fwereade has suggested previously, this can be shortened to: resp := struct{ Flavors []nova.FlavorDetail `json:"flavors"` }{flavors} https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:331: if len(flavors) == 0 { this might be better further up, as if len(flavors) == 0 { flavors = []nova.FlavorDetail{} } https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:348: panic("unknown request method: " + r.Method) could probably just return an error here rather than panicing. also, should probably lose the default clause and return the error instead of nil below. same applies to other handle functions below. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:359: defer r.Body.Close() this is unnecessary, i believe, as it's closed automatically when the handler returns. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:378: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:388: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:398: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:411: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:424: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:469: var resp struct { use shorter form as suggested above? https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:491: defer r.Body.Close() d https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:493: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:510: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:529: var resp struct { shorter form? https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:558: // response was sent in this case) i think that if no group is found in the path, this method should still return an error (say errNoGroupId ?). // processGroupId returns the group id from the given request. // If there was no group id specified in the path, it returns errNoGroupId. i don't think there's a need to pass in the ResponseWriter any more. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:578: if group, err := n.processGroupId(w, r); group != nil { i think i'd process this slightly more conventionally: group, err := n.processGroupId(w, r) if err == errNoGroupId { groups := n.allSecurityGroups() ... return } if err != nil { return err } send single group response https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:602: defer r.Body.Close() d https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:613: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:623: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:627: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:641: if group, err := n.processGroupId(w, r); group != nil { same comments apply here as to the GET clause above. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:678: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:684: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:688: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:764: return sendError(err, w, r) return err https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:797: handlers := map[string]http.Handler{ i think this looks loads better.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:40: "", On 2012/12/19 14:05:12, rog wrote: > all these errors should have meaningful error text (in case someone decides to > print them out, for logging or whatever). Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:129: errNoContent = &errorResponse{ On 2012/12/19 14:05:12, rog wrote: > we agreed online that this doesn't count as an error. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:135: errAccepted = &errorResponse{ On 2012/12/19 14:05:12, rog wrote: > as above. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:147: errInternal = &errorResponse{ On 2012/12/19 14:05:12, rog wrote: > since this isn't a single error like the others, but more a template for a class > of errors, i think this would probably be better crafted when an internal error > is created (see below). Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:172: if e.Error() != "" { On 2012/12/19 14:05:12, rog wrote: > i think this is wrong - all the err* values should have e.Error() != "". > if we inline the creation of errInternal (see below) i think we can lose this > logic. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:185: var body []byte On 2012/12/19 14:05:12, rog wrote: > perhaps: > body := e.requestBody() > > and rename replaceVars to requestBody and put the e.body != "" test into it. > > slightly less code and a better division of responsibilities perhaps? Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:198: On 2012/12/19 14:05:12, rog wrote: > in a future CL, it may be worth moving the meta-handler code into a separate > file from all the actual handlers. Ok. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:217: if err == nil { On 2012/12/19 14:05:12, rog wrote: > merge with previous line? No, because I need err further down. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:220: resp, _ := err.(http.Handler) On 2012/12/19 14:05:12, rog wrote: > these few lines have a few problems. > > i'd suggest something like this instead: > > if resp, _ := err.(http.Handler); resp != nil { > resp.ServeHTTP(w, r) > return > } > resp := &errorResp{ > ... details taken from errInternal > } > resp.ServeHTTP(w, r) Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:222: resp := errInternal On 2012/12/19 14:05:12, rog wrote: > this shadows the resp above, and hence will be ignored. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:223: resp.errorText = err.Error() On 2012/12/19 14:05:12, rog wrote: > this changes the global instance of errInternal, which probably isn't what you > want. > > to make it work, you should do something like: > iresp := *errInternal > iresp.errorText = err.Error() > resp = &iresp > but perhaps better just inlining the errInternal creation. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:230: func sendError(err error, w http.ResponseWriter, r *http.Request) error { On 2012/12/19 14:05:12, rog wrote: > i don't think we need this function any more Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:245: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:297: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:327: var resp struct { On 2012/12/19 14:05:12, rog wrote: > as fwereade has suggested previously, this can be shortened to: > > resp := struct{ > Flavors []nova.FlavorDetail `json:"flavors"` > }{flavors} Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:331: if len(flavors) == 0 { On 2012/12/19 14:05:12, rog wrote: > this might be better further up, as > if len(flavors) == 0 { > flavors = []nova.FlavorDetail{} > } Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:348: panic("unknown request method: " + r.Method) On 2012/12/19 14:05:12, rog wrote: > could probably just return an error here rather than panicing. > > also, should probably lose the default clause and return the error instead of > nil below. > > same applies to other handle functions below. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:359: defer r.Body.Close() On 2012/12/19 14:05:12, rog wrote: > this is unnecessary, i believe, as it's closed automatically when the handler > returns. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:378: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:388: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:398: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:411: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:424: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:469: var resp struct { On 2012/12/19 14:05:12, rog wrote: > use shorter form as suggested above? Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:491: defer r.Body.Close() On 2012/12/19 14:05:12, rog wrote: > d Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:493: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:510: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:529: var resp struct { On 2012/12/19 14:05:12, rog wrote: > shorter form? Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:558: // response was sent in this case) On 2012/12/19 14:05:12, rog wrote: > i think that if no group is found in the path, this method should still return > an error (say errNoGroupId ?). > > // processGroupId returns the group id from the given request. > // If there was no group id specified in the path, it returns errNoGroupId. > > i don't think there's a need to pass in the ResponseWriter any more. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:578: if group, err := n.processGroupId(w, r); group != nil { On 2012/12/19 14:05:12, rog wrote: > i think i'd process this slightly more conventionally: > > group, err := n.processGroupId(w, r) > if err == errNoGroupId { > groups := n.allSecurityGroups() > ... > return > } > if err != nil { > return err > } > send single group response Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:602: defer r.Body.Close() On 2012/12/19 14:05:12, rog wrote: > d Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:613: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:623: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:627: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:641: if group, err := n.processGroupId(w, r); group != nil { On 2012/12/19 14:05:12, rog wrote: > same comments apply here as to the GET clause above. Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:678: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:684: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:688: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:764: return sendError(err, w, r) On 2012/12/19 14:05:12, rog wrote: > return err Done. https://codereview.appspot.com/6940073/diff/6001/testservices/novaservice/ser... testservices/novaservice/service_http.go:797: handlers := map[string]http.Handler{ On 2012/12/19 14:05:12, rog wrote: > i think this looks loads better. I like it much better!
Sign in to reply to this message.
LGTM with the following points addressed. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:40: "Unauthorized request", error messages are all lower case by convention. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:77: "Not found plain body", "resource not found" ? https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:83: "Not found JSON body", "resource not found" (i don't think it needs a different error string really; if so, then "resource not found (JSON format)" would be fine) https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:95: "Not found security rule id", "security rule not found" https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:105: "Multiple URL redirection choices ", s/ "/"/ https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:149: // requestBody replaces $ENDPOINT$, $URL$, $ID$, and $ERROR$ in the // requestBody returns the body for the error response, // replacing $ENDPOINT$, $URL$, $ID$, and $ERROR$ // in e.body with values from the request. ? https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:216: func sendCode(code int, w http.ResponseWriter, r *http.Request) error { how about: func writeResponse(w http.ResponseWriter, code int, body []byte) { // workaround for https://code.google.com/p/go/issues/detail?id=4454 w.Header().Set("Content-Length", strconv.Itoa(len(data))) w.WriteHeader(code) w.Write(body) } ? then we can use it in sendJSON and errorResponse.ServeHTTP too, and we've got the workaround in one place only. BTW we don't usually make functions that always return a nil error just to avoid an extra line. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:230: if resp != nil { i don't think this logic is necessary. we never send json with a nil resp AFAICS. just data, err := json.Marshal(resp) if err != nil { return err } should be sufficient, i reckon. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:235: // workaround for https://code.google.com/p/go/issues/detail?id=4454 writeResponse(w, code, data) https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:365: if err = n.addServerSecurityGroup(server.Id, group.Id); err != nil { s/=/:=/ it's more conventional, when we don't care about err outside the if block and it's easy to do. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:543: if group, err := n.processGroupId(w, r); group != nil { please process the error cases first, as suggested. it's a common pattern and lessens the "what's going on here" factor. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:574: if err = json.Unmarshal(body, &req); err != nil { s/=/:=/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:40: "Unauthorized request", On 2012/12/19 17:11:37, rog wrote: > error messages are all lower case by convention. Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:77: "Not found plain body", On 2012/12/19 17:11:37, rog wrote: > "resource not found" > ? Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:83: "Not found JSON body", On 2012/12/19 17:11:37, rog wrote: > "resource not found" > (i don't think it needs a different error string really; if so, then "resource > not found (JSON format)" would be fine) Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:95: "Not found security rule id", On 2012/12/19 17:11:37, rog wrote: > "security rule not found" Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:105: "Multiple URL redirection choices ", On 2012/12/19 17:11:37, rog wrote: > s/ "/"/ Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:149: // requestBody replaces $ENDPOINT$, $URL$, $ID$, and $ERROR$ in the On 2012/12/19 17:11:37, rog wrote: > // requestBody returns the body for the error response, > // replacing $ENDPOINT$, $URL$, $ID$, and $ERROR$ > // in e.body with values from the request. > > ? Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:216: func sendCode(code int, w http.ResponseWriter, r *http.Request) error { On 2012/12/19 17:11:37, rog wrote: > how about: > > func writeResponse(w http.ResponseWriter, code int, body []byte) { > // workaround for https://code.google.com/p/go/issues/detail?id=4454 > w.Header().Set("Content-Length", strconv.Itoa(len(data))) > w.WriteHeader(code) > w.Write(body) > } > ? > > then we can use it in sendJSON and errorResponse.ServeHTTP too, and we've got > the workaround in one place only. > > BTW we don't usually make functions that always return a nil error just to avoid > an extra line. Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:230: if resp != nil { On 2012/12/19 17:11:37, rog wrote: > i don't think this logic is necessary. we never send json with a nil resp > AFAICS. > just > > data, err := json.Marshal(resp) > if err != nil { > return err > } > > should be sufficient, i reckon. Not anymore - sendJSON before I used like now writeResponse. Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:235: // workaround for https://code.google.com/p/go/issues/detail?id=4454 On 2012/12/19 17:11:37, rog wrote: > writeResponse(w, code, data) Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:365: if err = n.addServerSecurityGroup(server.Id, group.Id); err != nil { On 2012/12/19 17:11:37, rog wrote: > s/=/:=/ > > it's more conventional, when we don't care about err outside the if block and > it's easy to do. Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:543: if group, err := n.processGroupId(w, r); group != nil { On 2012/12/19 17:11:37, rog wrote: > please process the error cases first, as suggested. it's a common pattern and > lessens the "what's going on here" factor. Done. https://codereview.appspot.com/6940073/diff/15001/testservices/novaservice/se... testservices/novaservice/service_http.go:574: if err = json.Unmarshal(body, &req); err != nil { On 2012/12/19 17:11:37, rog wrote: > s/=/:=/ Done.
Sign in to reply to this message.
just a couple of tinies, but LGTM with those addressed. https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... testservices/novaservice/service_http.go:214: func writeResponse(code int, body []byte, w http.ResponseWriter, r *http.Request) { i'd put w first here (it's like the receiver of the behaviour - same order as w.Write(code, data)), and we can lose the request parameter - it's not used. https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... testservices/novaservice/service_http.go:222: // If JSON mashaling fails, an error is returned. marshaling but i think this line can go. it's not useful info, and it's trivial to see if we look at the function body.
Sign in to reply to this message.
*** Submitted: nova double: Rest of HTTP API. This implements the rest of the HTTP API of the Nova testing double: * servers * servers/detail * servers/<id>/os-security-groups * servers/<id>/action (add/removeSecurityGroup; add/removeFloatingIp) * os-security-groups * os-security-group-rules * os-floating-ips There are a few things to change in order to integrate with the Nova client and the local "live" tests, but this will come in a follow-up. R=fwereade, rog, jameinel CC= https://codereview.appspot.com/6940073 https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... testservices/novaservice/service_http.go:214: func writeResponse(code int, body []byte, w http.ResponseWriter, r *http.Request) { On 2012/12/19 17:43:43, rog wrote: > i'd put w first here (it's like the receiver of the behaviour - same order as > w.Write(code, data)), and we can lose the request parameter - it's not used. Done. https://codereview.appspot.com/6940073/diff/19001/testservices/novaservice/se... testservices/novaservice/service_http.go:222: // If JSON mashaling fails, an error is returned. On 2012/12/19 17:43:43, rog wrote: > marshaling > > but i think this line can go. it's not useful info, and it's trivial to see if > we look at the function body. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
