-
Notifications
You must be signed in to change notification settings - Fork 813
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
Remove env namespace dependency from allocator service. #814
Conversation
Build Failed 😱 Build Id: 7fd6c7b0-a29b-41c7-8b31-e2c05b81cea3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
7189d26
to
7072852
Compare
Build Succeeded 👏 Build Id: 1882d22b-5176-4677-95cb-77d3c8995dfa The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: a6d052cc-0b41-4d13-bf61-6cf1c7b25ce2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
test/e2e/allocator_test.go
Outdated
@@ -80,7 +83,9 @@ func TestAllocator(t *testing.T) { | |||
response, err = client.Post(requestURL, "application/json", bytes.NewBuffer(body)) | |||
|
|||
if err != nil { | |||
response.Body.Close() // nolint: errcheck | |||
if response != nil { | |||
response.Body.Close() // nolint: errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Small Go gotcha. response.Body.Close() should always get run if the response not nil. Usually defer
is a nice way to do it.
Details, and examples: https://stackoverflow.com/questions/33238518/what-could-happen-if-i-dont-close-response-body-in-golang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response.Body.Close() is happening after the poll. So this one was not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! Nice!
f1be9f6
to
c59120a
Compare
Build Failed 😱 Build Id: 9f43c177-0f81-4dff-8b85-9fb320896ee6 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: c63f1750-3b8b-4b8b-92c6-58532e467466 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 awesome.
Build Failed 😱 Build Id: c5cc0c4d-ebdf-406c-a0c1-d74fa0b830f0 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: eae0f125-3e73-4168-8e16-d148e2cfe4e3 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 28372f6b-c81b-4206-a607-75bfadd850ca To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
So this is consistently failing on this flaky test:
I'll rerun this once #812 has been merged. That should solve this problem and let this pass. |
Build Succeeded 👏 Build Id: 097ac65f-379c-4b7d-ac6f-0ad47ea7e336 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
issue #809