Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update golangci-lint to 1.18, add bodyclose check #1051

Merged
merged 1 commit into from
Sep 20, 2019
Merged

Update golangci-lint to 1.18, add bodyclose check #1051

merged 1 commit into from
Sep 20, 2019

Conversation

aLekSer
Copy link
Collaborator

@aLekSer aLekSer commented Sep 10, 2019 •
edited
Loading

Fix some tests which leak connections (bodyclose linter).

This version of golangci-lint would be needed for upgrading to Golang 1.13.

Fixed also:

pkg/util/apiserver/apiserver.go:128:22: appendAssign: append result not assigned to the same slice (gocritic)                                                                                
        list.APIResources = append(as.resourceList[groupVersion].APIResources, resource)

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 834fbf51-0b51-4fb1-8fed-bcb92b9b2ec5

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1051/head:pr_1051 && git checkout pr_1051
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.0.0-bab7841

@markmandel markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Sep 10, 2019
roberthbailey
roberthbailey reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

Since all of the missing closes were in tests, I think we should hold this PR until after the 1.0 release is cut.

test/e2e/allocator_test.go Outdated
@@ -82,7 +82,7 @@ func TestAllocator(t *testing.T) {
return true, err
}

response, err = client.Post(requestURL, "application/json", bytes.NewBuffer(body))
response, err = client.Post(requestURL, "application/json", bytes.NewBuffer(body)) // nolint: bodyclose
Copy link
Member

Choose a reason for hiding this comment

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

it seems like this one should be fixed to actually close the body.

Copy link
Collaborator Author

@aLekSer aLekSer Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I got an issue with golangci-lint 1.17.1 had false positive on that line. Will check with 1.18 version from this PR, they have updated bodyclose.

pkg/util/apiserver/apiserver.go Outdated
@@ -108,13 +108,12 @@ func NewAPIServer(mux *http.ServeMux) *APIServer {
// e.g. http://localhost:8001/apis/scheduling.k8s.io/v1beta1
// as well as registering a CRDHandler that all http requests for the given APIResource are routed to
func (as *APIServer) AddAPIResource(groupVersion string, resource metav1.APIResource, handler CRDHandler) {
list, ok := as.resourceList[groupVersion]
_, ok := as.resourceList[groupVersion]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the changes in this file. Should this be included? It doesn't have anything to do with fixing the lint errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file was included because it contains gocritic appendAssign issue. Well I can move it into another PR. I mentioned this file in description to this PR. Different variables in the left side and in the parameters to append.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing this change from PR.

@markmandel markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Sep 17, 2019
@roberthbailey
Copy link
Member

Now that 1.0 has been cut, can you address comments so that we can get this merged? Thanks!

@roberthbailey roberthbailey self-assigned this Sep 19, 2019
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b57a4f6e-0c23-440d-a38e-1e584c278da7

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1051/head:pr_1051 && git checkout pr_1051
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-3b9f33c

@aLekSer
Update golangci-lint to 1.18, add bodyclose check
6deb5b9 
Fix some test which leaks connections.
@aLekSer
Copy link
Collaborator Author

aLekSer commented Sep 20, 2019 •
edited
Loading

There was a need to rewrite test/e2e/allocator_test.go. Added defer and moved code reading from Body to wait.PollImmediate(). Code is equivalent of what we had, but with defer response.Body.Close().

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e89736b6-a1af-4f23-b52f-92f26f355463

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1051/head:pr_1051 && git checkout pr_1051
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-6deb5b9

markmandel
markmandel approved these changes Sep 20, 2019
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

👍

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aLekSer, markmandel
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markmandel markmandel merged commit fa69d11 into googleforgames:master Sep 20, 2019
@markmandel markmandel added this to the 1.1.0 milestone Oct 22, 2019
@markmandel markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc labels Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey left review comments

@markmandel markmandel markmandel approved these changes

@dzlier-gcp dzlier-gcp Awaiting requested review from dzlier-gcp

Assignees

@markmandel markmandel

@roberthbailey roberthbailey

Labels
area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc lgtm size/S
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@aLekSer @agones-bot @roberthbailey @google-oss-robot @markmandel