Skip to content

Update go-swagger libraries to the latest version and update to go modules #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 28, 2019

Conversation

bcomnes
Copy link
Contributor

@bcomnes bcomnes commented Feb 28, 2019

This PR turned into a mess. It does a few things:

Fix the missing consumer errors

The go-swagger runtime crashed on plain/html content types. This was causing problems in the buildbot, so the first step to address that was to update the go-swagger runtime libs to get the following fix:

Update go-swagger to v0.18.0 from 94927f8c9742791a9aa46c6c3965f0a37025f41d

Fix the failing tests

After updating the go-swagger libraries, this test started failing:

func TestRetryableTransport_POST(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
rw.WriteHeader(http.StatusCreated)
rw.Write([]byte("test result"))
}))
defer server.Close()
rwrtr := runtime.ClientRequestWriterFunc(func(req runtime.ClientRequest, _ strfmt.Registry) error {
return req.SetBodyParam("test result")
})
hu, _ := url.Parse(server.URL)
rt := NewRetryableTransport(httptransport.New(hu.Host, "/", []string{"http"}), 2)
result, err := rt.Submit(&runtime.ClientOperation{
ID: "createSite",
Method: "POST",
PathPattern: "/",
Params: rwrtr,
Reader: runtime.ClientResponseReaderFunc(func(response runtime.ClientResponse, consumer runtime.Consumer) (interface{}, error) {
if response.Code() == 201 {
var result string
if err := consumer.Consume(response.Body(), &result); err != nil {
return nil, err
}
return result, nil
}
return nil, errors.New("Generic error")
}),
})
require.NoError(t, err)
actual := result.(string)
require.EqualValues(t, "test result", actual)

The error was the following:

Post http://127.0.0.1:41076/: http: ContentLength=14 with Body length 28

This comes from deep within the http request client, and happens when the body exceeds the content length of the req.ContentLength property.

https://github.com/golang/go/blob/61170f85e62f1326d42c4dbd8aa17ab4a1305a87/src/net/http/transfer.go#L387-L389

Rolling back to [email protected] from [email protected] makes the test pass again, but we should really confirm retry's with a body work.

Whats happening is:

  • In the newer autorest versions, it calls rr.req.GetBody()

https://github.com/Azure/go-autorest/blob/1f7cd6cfe0adea687ad44a512dfe76140f804318/autorest/retriablerequest_1.8.go#L47-L53

  • This doubles the size of the body in the req object for some reason. I haven't dug into why that is yet.
  • For comparison: here is the new implementation of GetBody we are stuck with for now that seems to double the size body's size:

https://github.com/go-openapi/runtime/blob/9c75b2b632f58f9cb20e0767a724d5ebc37c2047/client/request.go#L209-L220

  • Here is the old implementation of GetBody

https://github.com/go-openapi/runtime/blob/94927f8c9742791a9aa46c6c3965f0a37025f41d/client/request.go#L249-L254

  • For comparison here are the two other strategies:

https://github.com/Azure/go-autorest/blob/1f7cd6cfe0adea687ad44a512dfe76140f804318/autorest/retriablerequest_1.7.go

  • (this is the version we effectively rolled back too)

https://github.com/Azure/go-autorest/blob/1f7cd6cfe0adea687ad44a512dfe76140f804318/autorest/retriablerequest.go

Port over to go-modules

I simultaneously tried to move the go module over to its own repo https://github.com/netlify/go-client/tree/open-api but that is still being discussed: netlify/go-client#17

I wanted to get that work out asap since the buildbot is already on go-modules, so I ported that work into this PR.

Add test to cover no consumer errors

Add a tests to verify the client doesn't error out when it receives an html or text response.

Closes #122
Closes netlify/buildbot#362

@bcomnes bcomnes self-assigned this Feb 28, 2019
@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

Same error as #147, but with the new toolchain.

@bcomnes bcomnes changed the title Switch to go modules and version toolchain Update go-swagger libraries to the latest version and update to go modules Feb 28, 2019
@bcomnes bcomnes mentioned this pull request Feb 28, 2019
@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

What's left now is to write some retriable tests with a body.

@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

Ok, end of the day here for me, for tomorrow, I think its the interaction between:

@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

Adding a replay test with the pinned version of autorest seems to work, so thats promising.

@bcomnes bcomnes requested a review from calavera February 28, 2019 03:09
@bcomnes bcomnes added the wip label Feb 28, 2019
@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

The docs changes here are still a wip.

This reproduces the no-consumer error seen in failing buildbot errors.  Since we updated the go-swagger libraries, this is no longer causes requests to fail.    

Closes #122
Closes https://github.com/netlify/buildbot/issues/362
@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 28, 2019

Added a test to confirm we no longer will get the no-consumer error when we get an unexpected content type.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bcomnes bcomnes merged commit 8ea1da5 into master Feb 28, 2019
@bcomnes bcomnes deleted the go-modules branch February 28, 2019 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle 408 timeouts better in go client / text error decoder
2 participants