Switch linter from gometalinter to golangci-lint

[corneliusweig]

Close #174
Based on #178

[codecov-io]

Codecov Report

Merging #176 into master will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master     #176   +/-   ##
=======================================
  Coverage   50.91%   50.91%           
=======================================
  Files          13       13           
  Lines         711      711           
=======================================
  Hits          362      362           
  Misses        297      297           
  Partials       52       52

Impacted Files	Coverage Δ
pkg/environment/environment.go	77.27% <ø> (ø)	⬆️
pkg/version/version.go	100% <ø> (ø)	⬆️
pkg/installation/upgrade.go	0% <0%> (ø)	⬆️
pkg/installation/install.go	35.16% <100%> (ø)	⬆️
pkg/download/downloader.go	68.04% <100%> (ø)	⬆️
pkg/installation/move.go	32.55% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ac6aa0...1442b8a. Read the comment docs.

[ahmetb]

I think the commits from other rename PR got in here, too. Please rebase once the other one merges.

[corneliusweig]

@ahmetb done

[ahmetb]

Thanks for proposing this. I feel a bit uneasy about hack/install_golangci-lint.sh which is auto-generated, long and doesn't seem maintainable.

I think we shouldn't use megalinters like this one either. I'm not sure if the value add is enough.

I'm trying to understand what other kubernetes projects use. Here's a search query: https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/kubernetes+%5Cblint+-file:vendor

I suspect falling back to x/lint and go vet for now would suffice. What do you think of this?

[ahmetb]

x/lint seems fairly popular https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/kubernetes+%22golint+%22+-file:vendor+count:1000

[corneliusweig]

I'm not sure I understand your concerns about a metalinter, after all gometalinter was one too. I understand your worries about the generated script, though. I had another look at the golangci-lint docs and they offer an install script for CI environments. For example, kustomize also uses that https://github.com/kubernetes-sigs/kustomize/blob/540e4023dae93dc9d8fcab37b18734ef9df08339/.travis.yml#L31

Would that address your worries?

[corneliusweig]

However, this would mean that users can't run the linter locally without installing golangci-lint manually.

[ahmetb]

The part that annoyed me slightly is why there's a need for a very long general-purpose script (install_golangci-lint.sh) just to install a go binary?

Can't we just do wget+untar? I know their readme says "use this script" but I don't see the point just yet.

[corneliusweig]

Well I think the major argument for this script is convenience for users with different platforms and architectures. The script conveniently downloads the correct build for the current platform. As a new contributor, I would find it very nice if the linter would just work without having to manually download that binary from wherever that is.
Another issue is consistency of linter warnings. The reported issues do change slightly with each version. Pinning the linter version avoids confusion here.

Admittedly, the script is large. But it does have a purpose. (Plus it also checks the hashsum)

[ahmetb]

Yeah but we could write our own install script and it would download if the binary is not in $PATH from the pinned version. I suspect the script would be very short and maintainable if we wrote it.

Having wget/tar installed is a sensible expectation from any contributor so I’m not too worried about it. We already use sha256sum tooling in other scripts.

[corneliusweig]

Honestly, if we want to support contributors on different platforms, this script is the best way to go. After all, it's been tried and tested.

I feel you major concern is having the script in the repo. Would it be an option to run it as curl http:... | sh - and take the hosted file directly from github? I think it should also be possible to pin the version.

[ahmetb]

/lgtm

[ahmetb]

I don't know why this is a linter error. It’s pretty common coding practice to have package level-consts that are used throughout the code rather than duplicating it.

Another example is below. We should probably turn off this linter, it’s too aggressive.

[corneliusweig]

I don't quite understand what you mean. Travis and my machine think that the linter passes.

Do you mean that the constant should not be there and be inlined instead? This is what the linter complained about so I extracted the constants. If you think this is bad, I can see what linter needs to be disabled.

[ahmetb]

ah, wait, I now see that the linter is actually trying to help us.

that said, it's erroneous. the "unknown" string on both gitCommit() and gitTag() methods mean different things. so they should not be shared by the same variable. we can do:

const unknownGitCommit = "unknown"
const unknownGitTag = "unknown"

similarly, the tests below actually test against v != unknown which isn't good.

When we update the value of the const, we might be breaking the API (in this case we aren't), but the if test continues to test v != "unknown" literal string comparison, it could catch that.

So I think we should still disable this particular linter.

[corneliusweig]

good catch. I disabled the goconst linter. I think it would have allowed two occurrences of "unknown" in tests, but goconst is not critical at all.

[corneliusweig]

@ahmetb can you take another look or close this PR?

[ahmetb]

Can you revert changes to this file? We since removed this linter.

[corneliusweig]

Sure. PTAL

[ahmetb]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

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:

• OWNERS [ahmetb]

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