Closed seh closed 7 years ago
I had forgotten to resolve the conflict here after #60 merged. PTAL.
fargo has been quiet for a while. Is anyone available to review this proposal? Maybe @ryansb or @cquinn? You two have been through these areas of the code. @damtur, by now, you know it all.
You are most welcome, @damtur.
If we're contemplating a major version bump, this might also be our one chance to fix these longstanding go lint violations:
dns_discover.go:15:5: exported var ErrNotInAWS should have comment or be unexported
dns_discover.go:79:21: error strings should not end with punctuation
dns_discover.go:92:6: func findDnsServerAddr should be findDNSServerAddr
dns_discover.go:98:9: if block ends with a return statement, so drop this else and outdent its block
errors.go:31:6: exported type AppNotFoundError should have comment or be unexported
net.go:31:9: if block ends with a return statement, so drop this else and outdent its block
net.go:686:45: method parameter insId should be insID
net.go:805:1: exported method Instance.Id should have comment or be unexported
net.go:805:20: method Id should be ID
rpc.go:14:5: exported var HttpClient should have comment or be unexported
rpc.go:14:5: var HttpClient should be HTTPClient
rpc.go:26:46: func parameter isJson should be isJSON
rpc.go:62:29: func parameter isJson should be isJSON
rpc.go:90:37: func parameter isJson should be isJSON
struct.go:27:2: struct field discoveryTtl should be discoveryTTL
struct.go:28:2: struct field UseJson should be UseJSON
struct.go:32:6: type GetAppsResponseJson should be GetAppsResponseJSON
struct.go:44:6: type GetAppResponseJson should be GetAppResponseJSON
struct.go:73:6: type RegisterInstanceJson should be RegisterInstanceJSON
struct.go:93:2: struct field HomePageUrl should be HomePageURL
struct.go:94:2: struct field StatusPageUrl should be StatusPageURL
struct.go:95:2: struct field HealthCheckUrl should be HealthCheckURL
struct.go:97:2: struct field CountryId should be CountryID
If you're interested in fixing those—breaking lots of callers in the process, yes—let me know and I'll file a separate PR with those renaming and documentation additions.
Hey @seh I think keeping migration process as simple as possible with this version bump should be the goal. I don't see enough value in changing those right now.
Weird: I thought I had responded to this 13 days ago when I also wrote a parallel reply:
One thing, which is bothering me still is that we should try to write really short note (maybe just links to the discussion we were having here) what has changed and how to migrate when someone's code doesn't compile.
Where would you like this documentation? Posted here in this proposal, or in note attached to a GitHub release, or maybe in the top-level README.md file?
I was waiting on your response before writing that documentation, and then I thought that maybe I had missed your reply. It turns out that I didn't post my question properly, despite me being sure that I did write it.
@seh I've merged the code now as it's good. I'll create a release while I get your note. Please write it anywhere so I have it and I will include it in a release note. Thank you for all your work!
Addressing both #58 and #29, this patch series started with the former and expanded to cover the latter as well. My testing confirms that fargo can communicate with a Eureka server of version era both pre-1.2.2 and post-1.2.2 using both XML and JSON, which is when the change in port value encoding came into service.
The challenges communicating with Eureka on each side of this version boundary involve several fields in the JSON encoding for both instances and the application-level query response:
Pre-Eureka v1.2.2: string Post-Eureka v1.2.2: number
Pre-Eureka v1.2.2: string Post-Eureka v1.2.2: number
Pre-Eureka v1.2.2: number Post-Eureka v1.2.2: string
Furthermore, when registering instances with Eureka, post version 1.2.2 the server started requiring that JSON-encoded instances include the "dataCenterInfo.@class" field, so we have to synthesize that, while still accommodating custom data center types for which a caller will need to specify a class value.
Finally, I discovered that fargo couldn't decode an instance's data center metadata that contains values that look like numbers; the Eureka server will encode them as JSON numbers rather than strings, requiring us to decode them flexibly.
There are a few disparate solutions combined here, but I figured that they sit well enough together under the umbrella of better compatibility with the server. Again, I believe this patch series resolves the longstanding incompatibility with fargo, Eureka post-version 1.2.2, and JSON encoding. We should be able to both register and query applications and instances now.