[build] Enable golangci-lint #2766

linasm · 2020-10-19T06:23:23Z

What this PR does / why we need it:
WIP golangci-lint

Special notes for your reviewer:
Added depguardagainst imports:

      github.com/golang/protobuf/jsonpb: "replace with github.com/gogo/protobuf/jsonpb"
      google.golang.org/protobuf/encoding/protojson: "replace with github.com/gogo/protobuf/jsonpb"
      github.com/golang/protobuf/proto: "replace with github.com/gogo/protobuf/proto"
      google.golang.org/protobuf/proto: "replace with github.com/gogo/protobuf/proto"

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

This reverts commit 9201371.

.buildkite/pipeline.yml

.golangci.yml

mway

Looking good!

mway · 2020-11-04T14:07:04Z

.golangci.yml

+  lll:
+    # max line length, lines longer will be reported. Default is 120.
+    # '\t' is counted as 1 character by default, and can be changed with the tab-width option
+    line-length: 120


Let's go with 100. I'd rather use 80, honestly, but 100 seems like a good compromise.

Will change to 100.

mway · 2020-11-04T14:09:47Z

.golangci.yml

+    # XXX: if you enable this setting, unused will report a lot of false-positives in text editors:
+    # if it's called for subdir of a project it can't find funcs usages. All text editor integrations
+    # with golangci-lint call it on a directory with the changed file.
+    check-exported: false


Thoughts on defaulting this to true? We probably shouldn't allow unused exported identifiers except in special situations (and generally we should be referencing things we're using, even if only in tests), especially if we're trying to be intentional about API surface area.

does referencing the exported type from a test in the same package prevent the check from firing?

I think the false-positives from text editor integrations makes this a pain to enable.

What I'd like to avoid is blindly exporting types that aren't actually being used to avoid API bloat, such that we catch exported types that can't be proven to be used in some capacity (or otherwise, //nolint them).

I use vanilla vim (I know, I'm a masochist), so I can't comment on workarounds for editors, but off the cuff I'd rather have our linting be defined by goal state rather than by issues/limitations with editor(s).

@mway I agree that this would be a nice option in general.
But somewhat unexpetedly, after enabling it I'm getting messages on test functions everywhere:

(...) src/aggregator/aggregator/flush_mgr_test.go:36:6: func `TestFlushManagerReset` is unused (unused) src/dbnode/client/session_fetch_tagged_test.go:406:6: func `TestSessionFetchTaggedMergeWithRetriesTest` is unused (unused) src/query/graphite/common/basic_functions_test.go:97:6: func `TestNormalize` is unused (unused) (...)

Also, I believe we have a need to treat some pieces of this code base as a library, which would become complicated with this option. Leaving it at 'false` at least for now.

Ah, yeah, I see what you mean. If we were able to skip test files for that specific linter, that seems like it would help, but if we're just going to get a bunch of noise because it doesn't understand that they're tests, agree that this isn't great.

I wonder whether it behaves differently in _test packages, which I think we should be using more broadly in general (although obviously not the case here in general, and probably wouldn't help us for the majority of the lint noise).

arnikola · 2020-11-04T20:22:30Z

Makefile

+lint: export GO_BUILD_TAGS = $(GO_BUILD_TAGS_LIST)
+lint: install-tools linter
+	@echo "--- :golang: Running linters"
+	./scripts/run-ci-lint.sh $(tools_bin_path)/golangci-lint
+	./bin/linter ./...


Can we make this able to run by subdir?

Good point, will do.

late to the party, but running by subdir multiplies the runtime by # of subdirs you have, and defeats most of golangci-lint optimizations (only parsing code once).
this should not be a default mode of operation.

Addressed in #2863

arnikola · 2020-11-04T20:29:12Z

.buildkite/pipeline.yml

@@ -35,8 +35,10 @@ steps:
        run: app
        workdir: /go/src/github.com/m3db/m3
    <<: *common
-  - name: "Services, Tools, Metalint"
-    command: make clean install-vendor-m3 services tools metalint


Are services and tools not relevant any more?

arnikola · 2020-11-04T20:29:18Z

.golangci.yml

+# options for analysis running
+run:
+  # default concurrency is a available CPU number
+  # concurrency: 4


Should we enable this? Don't suppose there's some way to take this from an env var so that we can run CI with high concurrency?

you can override this on the command line for CI

arnikola · 2020-11-04T20:31:43Z

.golangci.yml

+  skip-dirs:
+    - generated/.*
+    # There is some very weird golangci-lint bug that causes analysis to fail on the
+    # eventlog_server_test.go file (perhaps caused by https://github.com/golangci/golangci-lint/issues/995).


Did any of the suggestions in that thread help here?

arnikola · 2020-11-04T20:42:13Z

.golangci.yml

+  depguard:
+    list-type: blacklist
+    include-go-root: false
+    packages:


Can you add the other assert package that gets autoimported sometimes? I believe it's github.com/tj/assert, trying to remember if there's a similar issue for require but think it's ok

Good point, adding github.com/tj/assert.

arnikola · 2020-11-04T20:46:52Z

.golangci.yml

+    check-exported: false
+  nakedret:
+    # make an issue if func has more lines of code than this setting and it has naked returns; default is 30
+    max-func-lines: 30


nit; worth making this 1? I think we generally want to discourage naked returns considering we don't really use them anywhere and they're a lot harder to reason about

Reducing nesting, typically via early returns, is outlined in Uber's Go Style Guide:

Code should reduce nesting where possible by handling error cases/special conditions first and returning early or continuing the loop. Reduce the amount of code that is nested multiple levels.

Otherwise, we end up with a greater amount of indentation in aggregate, which isn't ideal.

arnikola · 2020-11-04T20:47:47Z

.golangci.yml

+    # True by default.
+    simple: true
+    range-loops: true # Report preallocation suggestions on range loops, true by default
+    for-loops: false # Report preallocation suggestions on for loops, false by default


Should we enable this one too?

arnikola · 2020-11-04T22:15:40Z

.golangci.yml

+    # Linter that checks that every comment ends in a period.
+    - godot


Does this only affect godoc or all comment? If it's godoc only may be worth enabling

arnikola

Looks good to me with nits on other linters to attempt

…/golangci-lint

linasm · 2020-11-05T11:24:49Z

@mway, @arnikola I've implemented most of your suggestions.
Given the usual controversies around linter settings (and trying to avoid "perfect is the enemy of done"), I suggest to land this, and continue any further discussion/fine tuning of specific settings as a separate effort.

mway

Thanks for pushing this through!

linasm added 9 commits October 19, 2020 09:21

[build] Enable golangci-lint

bb00d25

Buildkite

ac9766a

Increase lint timeout

d61a2a8

Try using new-from-rev

96e0c4e

Try to fail lint with a new change

9201371

A full .golangci.yml

54018f2

Revert "Try to fail lint with a new change"

e1e2164

This reverts commit 9201371.

Depguard against golang/protobuf imports

8857de6

More depguards

bd767c9

linasm changed the title ~~[build] Enable golangci-lint~~ WIP [build] Enable golangci-lint Oct 19, 2020

Merge branch 'master' into linasm/golangci-lint

2969bca

linasm mentioned this pull request Oct 19, 2020

Replace github.com/golang/protobuf/proto with github.com/gogo/protobuf/proto #2767

Open

linasm and others added 3 commits October 19, 2020 11:32

Fix

0cb5b4d

Merge branch 'master' into linasm/golangci-lint

1e2c5b2

Drop check_correct_grpc_dial

185c4c3

robskillington reviewed Oct 19, 2020

View reviewed changes

.buildkite/pipeline.yml Outdated Show resolved Hide resolved

robskillington reviewed Oct 19, 2020

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

linasm and others added 6 commits October 20, 2020 09:08

Try map[time.Time]string in CI

b29b669

Use ruleguard against map[time.Time]

a7f75dd

gci

2b78694

Remove metalint

9039fe7

Merge branch 'master' into linasm/golangci-lint

68c8824

Update new-from-rev

6f5ddf8

linasm marked this pull request as ready for review October 20, 2020 08:02

linasm changed the title ~~WIP [build] Enable golangci-lint~~ [build] Enable golangci-lint Oct 20, 2020

linasm assigned robskillington Oct 20, 2020

linasm mentioned this pull request Oct 26, 2020

[lint] Remove metalinter in favor of golangci-lint #2798

Closed

mway self-requested a review October 26, 2020 15:04

mway reviewed Oct 26, 2020

View reviewed changes

.golangci.yml Show resolved Hide resolved

Merge branch 'master' into linasm/golangci-lint

6cd7b07

linasm added 4 commits October 27, 2020 09:30

Enable additional linters

7e88ced

go mod tidy

5183a38

Copyrights

d81e64d

go mod tidy

745a77a

robskillington force-pushed the master branch from 834e25e to 99f874f Compare November 3, 2020 17:15

mway reviewed Nov 4, 2020

View reviewed changes

arnikola reviewed Nov 4, 2020

View reviewed changes

arnikola approved these changes Nov 4, 2020

View reviewed changes

linasm and others added 6 commits November 5, 2020 09:55

line-length: 100

15aa80b

Make lint a subdir target

b2a598b

Merge branch 'master' into linasm/golangci-lint

57da65a

Address (some) PR comments

bf24a80

Merge branch 'linasm/golangci-lint' of github.com:m3db/m3 into linasm…

52794fa

…/golangci-lint

go.mod/go.sum

72badac

mway approved these changes Nov 5, 2020

View reviewed changes

linasm added 2 commits November 5, 2020 17:11

Fix buildkite step

c21d133

Fix buildkite step [2]

efc01df

linasm merged commit 5a3a12b into master Nov 5, 2020

linasm deleted the linasm/golangci-lint branch November 5, 2020 16:02

		# Linter that checks that every comment ends in a period.
		- godot

[build] Enable golangci-lint #2766

[build] Enable golangci-lint #2766

Uh oh!

Conversation

linasm commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnikola Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnikola left a comment

Choose a reason for hiding this comment

Uh oh!

linasm commented Nov 5, 2020

Uh oh!

mway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linasm commented Oct 19, 2020 •

edited

Loading

arnikola Nov 4, 2020 •

edited

Loading