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

ci: reenable goerr113 and unused linters across the codebase #21578

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Oct 4, 2022

Does what it says on the tin. Now the privileged_tests tags in test files are gone, the unused linter can be useful again. Remove all code it flagged.

Two false positives were spotted: functions that are only used in Benchmark___ functions. There have been a few bugs around this with Example_ and Test_, but since it's just a few funcs for now, just //nolint:unused them.

ci: reenable goerr113 and unused linters across the codebase

Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo requested review from a team as code owners October 4, 2022 13:29
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 4, 2022
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks 💯

@ti-mo ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label Oct 4, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 4, 2022
@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 4, 2022

/test

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM from my codeowners, just a couple of comments where maybe the right answer is to add the piece of logic that should have been using this code rather than deleting the code.

@@ -900,104 +900,6 @@ func testExternalTrafficPolicyLocal(kubectl *helpers.Kubectl, ni *helpers.NodesI
}
}

func testHostPort(kubectl *helpers.Kubectl, ni *helpers.NodesInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

@cilium/sig-datapath do we have coverage of this now? I'm surprised this was unused.

Copy link
Contributor

@joamaki joamaki Oct 5, 2022

Choose a reason for hiding this comment

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

Looks like this wasn't cleaned up as part of the tests. We now have test/controlplane/services/nodeport testing that we populate lbmap correctly for HostPort and https://github.com/cilium/cilium-cli/blob/master/connectivity/tests/host.go tests e2e, so we should be good.

daemon/cmd/kube_proxy_replacement_test.go Show resolved Hide resolved
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM pending Joe's comments

@ti-mo
Copy link
Contributor Author

ti-mo commented Oct 5, 2022

All relevant tests passing, merging.

@ti-mo ti-mo added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 5, 2022
@ti-mo ti-mo merged commit d94c690 into cilium:master Oct 5, 2022
@ti-mo ti-mo deleted the tb/reenable-unused-linter branch October 5, 2022 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants