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.

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

🌱 Update golangci-lint 1.50.1, add more linters #2480

Merged
merged 26 commits into from
Jan 5, 2023
Merged

🌱 Update golangci-lint 1.50.1, add more linters #2480

merged 26 commits into from
Jan 5, 2023

Conversation

vincepri
Copy link
Contributor

Summary

This PR updates golanci-lint, adds many new linters, and fixes up a number of linter-found issues. For simplicity of review, the commits are split into multiple ones, although happy to squash them if needed.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 13, 2022
@sttts
Copy link
Member

sttts commented Dec 13, 2022

Can we hold that until the workspace refactoring has merged? Fearing rebase hell.

@vincepri
Copy link
Contributor Author

@sttts Absolutely, I forgot to put the hold after talking with Andy, I'll wait and do a rebase on your refactoring once it's ready

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2022
Makefile Show resolved Hide resolved
cmd/cache-server/main.go Outdated Show resolved Hide resolved
test/e2e/framework/accessory.go Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 23, 2022
@vincepri
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 30, 2022
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2023
@vincepri
Copy link
Contributor Author

vincepri commented Jan 3, 2023

Let me know if we'd prefer to have a single commit here, or keep them separate, happy to do either way

@vincepri
Copy link
Contributor Author

vincepri commented Jan 3, 2023

/retest

@stevekuznetsov
Copy link
Contributor

Don't think multiple commits will hurt anything - re-triggering the tests to clear flakes

@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2023
@@ -56,12 +56,12 @@ func WithShardNameFromContextRoundTripper(cfg *rest.Config) *rest.Config {
// It changes the URL path to target a shard from the context.
//
// For example given "amber" shard name in the context it will change
// apis/apis.kcp.io/v1alpha1/apiexports to /shards/amber/apis/apis.kcp.io/v1alpha1/apiexports
// apis/apis.kcp.dev/v1alpha1/apiexports to /shards/amber/apis/apis.kcp.dev/v1alpha1/apiexports.
Copy link
Member

Choose a reason for hiding this comment

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

undo

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2023
pkg/cache/server/handler.go Outdated Show resolved Hide resolved
if err != nil {
klog.V(5).Infof("Can not create request %v", err)
return nil, err
}

klog.V(5).Infof("Listener creating connection to %s", connect)
res, err := ln.client.Do(req)
res, err := ln.client.Do(req) //nolint:bodyclose // Seems we're returning te connection with res.Body, caller closes it?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
res, err := ln.client.Do(req) //nolint:bodyclose // Seems we're returning te connection with res.Body, caller closes it?
res, err := ln.client.Do(req) //nolint:bodyclose // Seems we're returning the connection with res.Body, caller closes it?

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2023
@ncdc
Copy link
Member

ncdc commented Jan 5, 2023

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2023
@openshift-merge-robot openshift-merge-robot merged commit d336e53 into kcp-dev:main Jan 5, 2023
@vincepri vincepri deleted the update-golanci-lint2 branch January 5, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants