Move to golangci-lint #1853

bboreham · 2019-11-25T15:43:23Z

This is faster and does more checks.

Remove home-grown lint script.
Install the golangci-lint binary in the build container.
Remove individual tools gocyclo, golint, errcheck which are included in golangci-lint.

Run in --new-from-rev mode so it doesn't fail the build on a bunch of warnings that were pre-existing.

Still running go vet separately because I couldn't see that the vet tool inside golangci-lint is the same thing.

Also run misspell specifically on the docs because golangci-lint only does *.go.

Also remove a couple of unnecessary steps from the build image.

khaines

I've seen the golangci tools used in other projects, so if it improves our build process in this project, i'm all for it!

jtlisi

LGTM, love golangci-lint

pracucci · 2019-11-26T10:18:40Z

Makefile

-	go vet -stdmethods=false ./pkg/...
+	go vet -tags netgo -stdmethods=false ./pkg/...
+	misspell -error docs
+	golangci-lint run --new-from-rev ed7c302fd968 --build-tags netgo --timeout=5m --enable golint --enable misspell --enable gofmt


This is particularly slow when running on OSX (with Docker for Mac) due to the shared volumes performances. Do you see any problem adding :delegated to the 3 related volumes, switching

@$(SUDO) time docker run $(RM) $(TTY) -i \ -v $(shell pwd)/.cache:/go/cache \ -v $(shell pwd)/.pkg:/go/pkg \ -v $(shell pwd):/go/src/github.com/cortexproject/cortex \ $(BUILD_IMAGE) $@;

to

@$(SUDO) time docker run $(RM) $(TTY) -i \ -v $(shell pwd)/.cache:/go/cache:delegated \ -v $(shell pwd)/.pkg:/go/pkg:delegated \ -v $(shell pwd):/go/src/github.com/cortexproject/cortex:delegated \ $(BUILD_IMAGE) $@;

In my case, the execution time difference is:

Current: 8m8.369s

With delegated: 1m38.258s

What does :delegated do?
Would we want it on all similar lines?

What does :delegated do?

The :delegated tells Docker for Mac that the container’s view is authoritative (permit delays before updates on the container appear in the host). Details here: https://docs.docker.com/docker-for-mac/osxfs-caching/

I don't have a Linux box right now to play with, but it should just be ignored on Linux.

Would we want it on all similar lines?

I would say yes.

Have you considered adding :delegated to volume mounts?

Have done this now.

cyriltovena · 2019-11-27T14:49:26Z

Makefile

 	# -stdmethods=false disables checks for non-standard signatures for methods with familiar names.
 	# This is needed because the Prometheus storage interface requires a non-standard Seek() method.
-	go vet -stdmethods=false ./pkg/...
+	go vet -tags netgo -stdmethods=false ./pkg/...


I think go vet is on by default. We use it on Loki.

govet (vet, vetshadow): Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: true, auto-fix: false]

see https://github.com/golangci/golangci-lint#quick-start

I did this test: change a printf format string to invalid, e.g. %i.
I get a warning from go vet but not from golangci-lint.
So I am un-convinced.

Interesting I have the same issue in loki but if I ran it for the specific package it works. Tried to clean my cache also using golangci-lint cache clean.

This is failing golangci-lint run -v ./pkg/querier/queryrange

but not

golangci-lint run -v ./...

Found the issue on Loki side, one file was producing warning.

WARN [runner] Can't run linter goanalysis_metalinter: misspell: can't get file /Users/ctovena/go/src/github.com/grafana/loki/pkg/logql/pkg/logql/expr.y contents: can't read file /Users/ctovena/go/src/github.com/grafana/loki/pkg/logql/pkg/logql/expr.y: open /Users/ctovena/go/src/github.com/grafana/loki/pkg/logql/pkg/logql/expr.y: no such file or directory

This was caused by a comment //line expr.y, I cleanup the file and it now catches %i in fmt.

FYI it was also causing some linter to be skipped.

You have the same kind of warning.

WARN [runner] Can't run linter goanalysis_metalinter: fact_purity: failed prerequisites: [email protected]/cortexproject/cortex/pkg/chunk [github.com/cortexproject/cortex/pkg/chunk/cache.test]

Not sure why the exit code is not 1.

I found various issues relating to this - golangci/golangci-lint#866, golangci/golangci-lint#827

bboreham · 2019-12-10T20:02:47Z

I'm now running make lint twice in an attempt to work around golangci/golangci-lint#866, golangci/golangci-lint#827

Once I do that I am reassured that it does indeed run go vet so have taken that out of our commands.

bboreham · 2019-12-10T21:10:22Z

Rebased and we have a bunch of lint failures from code added since I started this PR!

I wonder if we should disable the return value check on log calls (or everywhere)?

pracucci · 2019-12-11T08:28:52Z

I wonder if we should disable the return value check on log calls (or everywhere)?

My personal take:

Definitely disable it for log calls
I still see a value for other use cases (errors should be checked/handled)

This is faster and does more checks. * Remove home-grown `lint` script. * Install the `golangci-lint` binary in the build container. * Remove individual tools `gocyclo`, `golint`, `errcheck` which are included in `golangci-lint`. * golangci-lint also runs go vet so we don't need to. Run in `--new-from-rev` mode so it doesn't fail the build on a bunch of warnings that were pre-existing. Also run `misspell` specifically on the docs because `golangci-lint` only does `*.go`. Signed-off-by: Bryan Boreham <[email protected]>

Go caches builds for different tags now, so we don't need to pre-build them. We don't use `dep` any more. Signed-off-by: Bryan Boreham <[email protected]>

Signed-off-by: Bryan Boreham <[email protected]>

bboreham · 2019-12-16T15:50:13Z

I figured out how to disable errcheck on individual methods.

pracucci · 2019-12-31T15:56:50Z

@bboreham What's the state of this PR? Is there anything missing?

bboreham · 2020-01-03T13:44:08Z

Ideally it would get an approval after the latest set of changes.

jtlisi

LGTM X2

khaines approved these changes Nov 25, 2019

View reviewed changes

jtlisi approved these changes Nov 25, 2019

View reviewed changes

pracucci reviewed Nov 26, 2019

View reviewed changes

cyriltovena reviewed Nov 27, 2019

View reviewed changes

cyriltovena mentioned this pull request Nov 27, 2019

Fixes linter warning from the yacc file. grafana/loki#1328

Merged

bboreham force-pushed the update-lint branch from beb54f1 to 96b633d Compare December 10, 2019 19:57

bboreham force-pushed the update-lint branch from 96b633d to 3940048 Compare December 10, 2019 20:02

jtlisi mentioned this pull request Dec 13, 2019

Pre Golang-CI Linting Errors #1912

Closed

bboreham force-pushed the update-lint branch from e3843d5 to 15a2856 Compare December 16, 2019 15:23

bboreham added 9 commits December 16, 2019 15:38

Remove some unnecessary steps from the build image

78be66f

Go caches builds for different tags now, so we don't need to pre-build them. We don't use `dep` any more. Signed-off-by: Bryan Boreham <[email protected]>

Run lint again because it fails the first time

f693322

Signed-off-by: Bryan Boreham <[email protected]>

Bit more DRY in Makefile

daf2596

Signed-off-by: Bryan Boreham <[email protected]>

Add :delegated to speed compilation on Mac

ed3ad4c

Signed-off-by: Bryan Boreham <[email protected]>

Add linter configuration to skip some checks

cfaa978

Signed-off-by: Bryan Boreham <[email protected]>

Fix some lint warnings in tests

9d7a794

Signed-off-by: Bryan Boreham <[email protected]>

Several code cleanups prompted by golangci-lint

6a79b84

Signed-off-by: Bryan Boreham <[email protected]>

Stop running 'go test -i' - Go caches builds itself now

ae47f74

Signed-off-by: Bryan Boreham <[email protected]>

bboreham force-pushed the update-lint branch from 15a2856 to efcec7e Compare December 16, 2019 15:41

Use build image from this branch

62d1042

Signed-off-by: Bryan Boreham <[email protected]>

bboreham force-pushed the update-lint branch from efcec7e to 62d1042 Compare December 16, 2019 15:50

jtlisi approved these changes Jan 8, 2020

View reviewed changes

bboreham merged commit 5528d67 into master Jan 8, 2020

bboreham deleted the update-lint branch January 8, 2020 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to golangci-lint #1853

Move to golangci-lint #1853

bboreham commented Nov 25, 2019

khaines left a comment

jtlisi left a comment

pracucci Nov 26, 2019

bboreham Nov 27, 2019

pracucci Nov 27, 2019

pracucci Dec 11, 2019

bboreham Dec 16, 2019

cyriltovena Nov 27, 2019

bboreham Nov 27, 2019

cyriltovena Nov 27, 2019

cyriltovena Nov 27, 2019

cyriltovena Nov 27, 2019

cyriltovena Nov 27, 2019

cyriltovena Nov 27, 2019

bboreham Dec 10, 2019

bboreham commented Dec 10, 2019

bboreham commented Dec 10, 2019

pracucci commented Dec 11, 2019

bboreham commented Dec 16, 2019

pracucci commented Dec 31, 2019

bboreham commented Jan 3, 2020

jtlisi left a comment

Move to golangci-lint #1853

Move to golangci-lint #1853

Conversation

bboreham commented Nov 25, 2019

khaines left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham commented Dec 10, 2019

bboreham commented Dec 10, 2019

pracucci commented Dec 11, 2019

bboreham commented Dec 16, 2019

pracucci commented Dec 31, 2019

bboreham commented Jan 3, 2020

jtlisi left a comment

Choose a reason for hiding this comment