Add golangci-lint to CI #114
Conversation
|
@cep21 build is broken which said it cannot parse .travis.yaml file. Could you check the format to make sure it can be built? |
.travis.yml
Outdated
| @@ -41,10 +41,12 @@ install: | |||
| else | |||
| go get; | |||
| fi | |||
| - [ $DEP_MANAGEMENT_TOOL == "DEP" ] || go get github.com/golangci/golangci-lint/cmd/[email protected] | |||
There was a problem hiding this comment.
I am wondering what you want to achieve here. Do you mean if $DEP_MANAGEMENT_TOOL == "DEP" and then go get ...?
There was a problem hiding this comment.
I'm not sure if we need to run a linter in every version of go, or just the latest version of Go. A lot of the linting tools are newer and only work on the latest version of Go.
I think it makes a lot of sense to run unit tests or integration tests on every version of Go, but unsure what advantage we get out of limiting linters to just those supported on 1.9.
There was a problem hiding this comment.
Oh I misunderstood your question!
I'm trying to install golangci-lint only if modules are enabled, which is only if we're using modules and not dep.
There was a problem hiding this comment.
in this case, we can make if else condition(there are some example inside the .travis.yaml file)
Ah! Where did you find that link (I cannot find this build on https://travis-ci.org/aws/aws-xray-sdk-go/pull_requests) |
|
@cep21 it is inside the Requests and here is the link: https://travis-ci.org/aws/aws-xray-sdk-go/requests |
There are lint failures that cause the linter to fail. We should try to continuously improve the codebase until we resolve these linter issues.
|
@cep21 thanks for the update and it works now. I am wondering you are you going to address the result of running the golangci-lint? |
|
There is a lot of output. Some don't matter and we can take out of the linter (like math/rand instead of crypto/rand ), but some are things we should resolve. I worry it would be too big a diff to do everything at once, but having the list at least in CI can give us a target to reach. What are your thoughts? |
|
@cep21 Yeah, I agree with you. A big diff can be too hard to read. Let's break down into several PRs to cover them. :) |
We should also task to improve these lint failures until the linter passes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.