Add JSON support: #6
Conversation
cquinn
commented
Jun 17, 2014
cquinn
commented
- rpc extensions to support both XML and JSON requests and responses
- net support for a Eureka connection using JSON or XML
- net support for marshalling and unmarshalling of the apps, app and instance structs
- custom unmarshallers and intermediate wrapper structs to deal with nasty Eureka JSON
- updated existing net tests to test both XML and JSON
- added new sample-driven tests for JSON unmarshalling. (tricky to get that working--tests were a lifesaver)
- rpc extensions to support both XML and JSON requests and responses - net support for a Eureka connection using JSON or XML - net support for marshalling and unmarshalling of the apps, app and instance structs - custom unmarshallers and intermediate wrapper structs to deal with nasty Eureka JSON
|
We have a need at our company (Riot Games) to use the JSON flavor of Eureka's REST API. This pull request adds that support to fargo in an optional way. I've made a few other changes that I'll comment on inline. Let me know if you are interested in this change in general, and I'll make sure that I've cleaned up anything you don't like. There are definitely some debug / comment bits in there that need some cleanup before committing. |
| @@ -0,0 +1,104 @@ | |||
| package fargo | |||
There was a problem hiding this comment.
This is where I put the custom JSON unmarshalling
|
I'd definitely be interested in this. Most of the reason there isn't JSON support already is the gotchas in the Eureka JSON API. I've made some comments, but this is definitely worth working on. Thanks a ton for your contributions. |
Split out instance retrieval needed in ReregisterInstance() into new functions GetInstance() and readInstanceInto() Small change to readAppInto() to match simpler form in readInstanceInto()
|
OK, I think I've addressed all your comments. |
| if err = json.Unmarshal(b, &ii); err == nil { | ||
| marshalLog.Debug("Instance.UnmarshalJSON ii:%+v\n", ii) | ||
| *i = Instance(ii) | ||
| i.Port, _ = strconv.Atoi(ii.PortJ.Number) |
There was a problem hiding this comment.
It's probably fine, but ignoring the err case here gives me pause. Maybe log if there's a problem?
|
Looks great. Just a couple nitpicks then I'll merge it and tag a new release. |
…king for port unmarshaling.
|
I think all your comments are addressed now. |
Add JSON support: (credit: @cquinn)
|
Great, thanks! |
