search

metaverse / truss

Truss helps you build go-kit microservices without having to worry about writing or maintaining boilerplate code.
Other
737 stars 144 forks source link

Do not send request body over HTTP(S) for GET requests. #273

Closed TyeMcQueen closed 5 years ago

TyeMcQueen commented 5 years ago

Not only is the body not needed for GET requests, including the body will cause such request to fail if they route through a GCP load balancer.

TyeMcQueen commented 5 years ago

This is a much simpler change and the failures look the same as for the prior PR and look unrelated to either PR. I suspect they fail the same way if rerun on master.

I'm going to try to repro one of the failures in my dev env by downgrading go because I can't make sense of how they are breaking.

FYI, I have more PRs in my pipe but I've wanted to send the fixes as small pieces to make them easy to understand, easy to accept, and allow for some items to not be accepted while still getting the others "done".

TyeMcQueen commented 5 years ago

Using go 1.10.8, I get no test failures.

Could this just be something broken in how travis is caching something?

zaquestion commented 5 years ago

It broken because of https://github.com/gogo/protobuf/commit/ce736d2dbdfbca236da3052a843a743b1e3eff13 which added a new struct and messed with a slightly somewhat over explicit test. 

--- FAIL: TestMessages (0.06s)
    newfromstring_test.go:119: got != want; methods differ: --- A

Can be ignore here and will get addressed in https://github.com/metaverse/truss/pull/274 once we work on the kinks.

Personally I loath when bodys getting sent along with GETs it's like you can't rely on anything in this world. Since this client is only for communicating with truss services, it seems appropriate to stop generating what is a clear REST violation that doesn't need to carry into new services.

zaquestion commented 5 years ago

@TyeMcQueen Can merge/rebase in the latest changes and that should resolve the tests.