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

Add 'nilness' to golangci #14066

Merged
merged 3 commits into from
Nov 23, 2020
Merged

Add 'nilness' to golangci #14066

merged 3 commits into from
Nov 23, 2020

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Nov 18, 2020

Nilness checks for common programming errors like nil pointer dereference or degenerate nil pointer comparisons. It can find bugs, add it to the golangci target.

https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/nilness

Add it to the CI, and fix up the couple of legitimate failures it complains about in the codebase.

Technically the two identified errors are bugs, but they do not appear to be at all severe so I see no strong motivation to backport this PR to v1.9.

@joestringer joestringer added the release-note/ci This PR makes changes to the CI. label Nov 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 18, 2020
nilness reports:

    # github.com/cilium/cilium/pkg/k8s/version
    pkg/k8s/version/version.go:278:9: impossible condition: nil != nil

Fix it by reusing the outer err variable.

CC: Deepesh Pathak <deepshpathak@gmail.com>
Fixes: fb101df ("k8s: add coordinationv1 capability check to k8s version package")
Signed-off-by: Joe Stringer <joe@cilium.io>
nilness complains that the local secret error is hidden:

    pkg/crypto/certificatemanager/certificate_manager.go:81:33:
      impossible condition: nil != nil

Fix it by unhiding the outer ioErr variable.

CC: Jarno Rajahalme <jarno@covalent.io>
Fixes: cabf83c ("crypto: Add local GetSecrets().")
Signed-off-by: Joe Stringer <joe@cilium.io>
Suggested-by: Tam Mach <sayboras@yahoo.com>
Signed-off-by: Joe Stringer <joe@cilium.io>
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.

Nice. I noticed in the fix commits, there's a CC to the original author. Is that some new fancy way in GH to notify the original author of these commits?

@joestringer
Copy link
Member Author

joestringer commented Nov 18, 2020

@christarazi actually it's part of my old git "fixes" formatting scripts:

https://github.com/joestringer/config/blob/1c050f3807e03cb8ba283c90b05416767f493576/.gitconfig#L8

https://github.com/joestringer/config/blob/1c050f3807e03cb8ba283c90b05416767f493576/.bashrc#L337-L366

@joestringer
Copy link
Member Author

Reassigned since it's probably more important to get the original authors to take a look both since they'll know that code and as a way to point out this pattern of bug to reduce recurrance of such issues.

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.

LGTM 💯

nit: Minor change in github PR title to reflect lastest approach will be great, commit messages look great 👍

Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks 🚀

@joestringer joestringer changed the title Add 'nilness' to github actions jobs Add 'nilness' to golangci Nov 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.1 Nov 19, 2020
@nathanjsweet
Copy link
Member

test-me-please

@joestringer joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 20, 2020
@joestringer
Copy link
Member Author

GKE only issue, but seems unlikely that it's any way connected to the changes in this PR:

https://jenkins.cilium.io/job/Cilium-PR-K8s-GKE/3342/execution/node/115/log/

09:17:01  WARNING: cluster cilium-ci-13 is not running. The kubernetes API may not be available.
09:17:01  ERROR: (gcloud.container.clusters.get-credentials) cluster cilium-ci-13 is missing endpoint. Is it still PROVISIONING?

@jibi jibi merged commit f3eacfb into cilium:master Nov 23, 2020
@joestringer joestringer deleted the submit/nilness branch November 23, 2020 20:06
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.9.1 Nov 23, 2020
@joestringer
Copy link
Member Author

No need to backport to 1.9, all future patches should go through master and therefore have these checks applied at the time of submission.

@joestringer
Copy link
Member Author

I also received out-of-band confirmation from @jrajahalme that these patches do not need backport to v1.9 as the outer code already handles such error conditions.

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/ci This PR makes changes to the CI.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants