Replace old linters with golangci-lint #3237

albertteoh · 2021-08-29T12:11:14Z

Which problem is this PR solving?

Migrate lint tools to golangci-lint #2503 and relates to Add golangci-lint #2602

Short description of the changes

Replaces our existing lint checks with golangci-lint.
Goal is to achieve parity (wrt linters) with the existing checks and minimal code changes. If we wish to include more linters, it's a straightforward config change.

Signed-off-by: Albert Teoh <[email protected]>

codecov · 2021-08-29T12:14:47Z

Codecov Report

Merging #3237 (8b3a050) into master (fb4be5d) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3237      +/-   ##
==========================================
- Coverage   95.99%   95.95%   -0.05%     
==========================================
  Files         242      242              
  Lines       14813    14813              
==========================================
- Hits        14220    14214       -6     
- Misses        514      519       +5     
- Partials       79       80       +1

Impacted Files	Coverage Δ
cmd/flags/admin.go	`82.81% <ø> (ø)`
cmd/collector/app/server/zipkin.go	`68.29% <0.00%> (-2.44%)`	⬇️
cmd/query/app/static_handler.go	`95.80% <0.00%> (-1.80%)`	⬇️
...lugin/sampling/strategystore/adaptive/processor.go	`99.35% <0.00%> (-0.65%)`	⬇️

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 fb4be5d...8b3a050. Read the comment docs.

Signed-off-by: Albert Teoh <[email protected]>

albertteoh · 2021-08-29T12:12:02Z

cmd/flags/admin.go

-	healthCheckHTTPPort = "health-check-http-port"
-	adminHTTPPort       = "admin-http-port"


Unused and deprecated here.

albertteoh · 2021-08-29T12:12:56Z

pkg/gogocodec/codec_test.go

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"


Please let me know if there are issues with moving to "google.golang.org/protobuf/proto", and I can revert back and add a //nolint directive. Tests appear to still be passing with this change.

albertteoh · 2021-08-29T12:14:45Z

Makefile

@@ -35,12 +35,7 @@ GOOS ?= $(shell go env GOOS)
 GOARCH ?= $(shell go env GOARCH)
 GOBUILD=CGO_ENABLED=0 installsuffix=cgo go build -trimpath
 GOTEST=go test -v $(RACE)
-GOLINT=golint


golint, go vet and gosec should no longer be needed as golangci-lint provides these linters, so I'm removing them from the Makefile to simplify.

albertteoh · 2021-08-29T12:15:08Z

.golangci.yml

+      - G104
+      - G107


These gosec rules were excluded in the Makefile, so I did the same.

albertteoh · 2021-08-29T12:18:46Z

.golangci.yml

+      - G404
+      - G601


These gosec rules were not excluded in the Makefile, but were identified, giving the errors:

cmd/collector/app/sanitizer/cache/auto_refresh_cache.go:136:40: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec) return (interval / 2) + time.Duration(rand.Int63n(int64(interval/2))) ^ crossdock/services/tracehandler.go:308:27: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec) return fmt.Sprintf("%x", rand.Int63()) ^ cmd/ingester/app/processor/decorator/retry.go:54:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec) rand: rand.New(rand.NewSource(time.Now().UnixNano())), ^ model/converter/thrift/jaeger/from_domain.go:106:37: G601: Implicit memory aliasing in for loop. (gosec) jaegerTags[idx] = d.keyValueToTag(&kv) ^ cmd/anonymizer/main.go:72:22: G601: Implicit memory aliasing in for loop. (gosec) writer.WriteSpan(&span)

I've excluded them for now, but if there is a preference to address these rules in this PR, I can do so.

let's fix them in another PR

albertteoh · 2021-08-29T13:16:07Z

.github/workflows/ci-unit-tests.yml

@@ -22,9 +22,15 @@ jobs:
    - name: Run unit tests
      run: make test-ci

+    - name: golangci-lint


I ended up placing the lint step within the unit-tests, which is similar behaviour to the Makefile. However, I feel it would be nicer as a separate job in order to enable concurrency, but am getting errors relating to undefined types. Oddly, testing on my fork doesn't have this issue.

I believe it's the same config as is with #2602 and does not appear to have encountered this problem. Maybe @daxmc99 has some ideas about this?

It might be worth noting that (I think) those "undefined" errors could be due to not having run go build on jaeger, which explains why moving this step as part of the unit tests job works.

yurishkuro · 2021-08-29T14:50:46Z

Makefile

-	$(MAKE) go-lint
-	@echo Running go fmt on ALL_SRC ...
-	@$(GOFMT) -e -s -l $(ALL_SRC) > $(FMT_LOG)
-	./scripts/updateLicenses.sh >> $(FMT_LOG)


This must remain

Nice catch, I've added this back into the lint target. Tested by removing the license from grpc_handler.go:

$ make lint golangci-lint run ./scripts/updateLicenses.sh > .fmt.log License check failures, run 'make fmt' cmd/collector/app/handler/grpc_handler.go make: *** [lint] Error 1

Makefile

yurishkuro · 2021-08-29T14:52:45Z

Makefile


 .PHONY: install-ci
 install-ci: install-tools

 .PHONY: test-ci
-test-ci: build-examples lint cover
+test-ci: build-examples cover


I would consider killing this target and invoking directly in GA

I like a minimal CI configuration. I want to be able to easily replicate locally what CI is doing - e.g. run the same make target.

Yup, I agree that it's a better developer experience if the CI scripts can be traced down to the make target then "replayed" locally, which a GA step doesn't afford.

As such, I've removed the GA step to run golangci-lint and kept the original setup where the test-ci target will run the lint target. Fewer changes, and cleaner.

Signed-off-by: Albert Teoh <[email protected]>

albertteoh · 2021-08-30T11:47:11Z

Makefile

 	go install github.com/mjibson/esc
-	go install github.com/securego/gosec/cmd/gosec
-	go install honnef.co/go/tools/cmd/staticcheck
+	go install github.com/golangci/golangci-lint/cmd/[email protected]


Add this to the tools so CI is able to run golangci-lint, and ensure devs also have the right tools installed to test linting.

albertteoh · 2021-08-30T11:53:35Z

.golangci.yml

+      - G404
+      - G601
+  gosimple:
+    go: "1.17"


Bumped the go version since we've moved onto 1.17 already.

albertteoh · 2021-09-01T21:45:26Z

Please let me know if there are any issues that still need addressing.

yurishkuro · 2021-09-02T00:10:53Z

Makefile

-	@echo Running go lint...
-	@$(GOLINT) $(ALL_PKGS) | grep -v _nolint.go >> $(LINT_LOG) || true;
-	@[ ! -s "$(LINT_LOG)" ] || (echo "Lint Failures" | cat - $(LINT_LOG) && false)
+	@[ ! -s "$(FMT_LOG)" -a ! -s "$(IMPORT_LOG)" ] || (echo "License check or import ordering failures, run 'make fmt'" | cat - $(FMT_LOG) && false)


This does not print import_log

Addressed in 8b3a050

Signed-off-by: Albert Teoh <[email protected]>

yurishkuro · 2021-09-02T04:58:14Z

.golangci.yml

+      - G404
+      - G601


let's fix them in another PR

albertteoh · 2021-09-02T05:09:31Z

thanks for the review Yuri/Pavol! 🙏🏼

albertteoh added 4 commits August 29, 2021 21:45

Add golangci-lint support

1e67b6d

Signed-off-by: Albert Teoh <[email protected]>

Merge branch 'master' into golangci-lint

f591546

Redundant

ceda4be

Signed-off-by: Albert Teoh <[email protected]>

Remove whitespace

c5b8e2d

Signed-off-by: Albert Teoh <[email protected]>

albertteoh added 2 commits August 29, 2021 22:38

Move golangci-lint into unit test workflow

5fb5d53

Signed-off-by: Albert Teoh <[email protected]>

Move golanci-lint step into Unit test job

7b1ab1c

Signed-off-by: Albert Teoh <[email protected]>

albertteoh commented Aug 29, 2021

View reviewed changes

albertteoh marked this pull request as ready for review August 29, 2021 13:17

albertteoh requested a review from a team as a code owner August 29, 2021 13:17

albertteoh requested a review from jpkrohling August 29, 2021 13:17

albertteoh changed the title ~~Golangci lint~~ Replace old linters with golangci-lint Aug 29, 2021

albertteoh mentioned this pull request Aug 29, 2021

Enable gzip compression for collector grpc endpoint. #3236

Merged

yurishkuro reviewed Aug 29, 2021

View reviewed changes

albertteoh added 5 commits August 30, 2021 21:05

Address review comments

39a4051

Signed-off-by: Albert Teoh <[email protected]>

Newline

3c23f85

Signed-off-by: Albert Teoh <[email protected]>

fmt -> lic

979b30b

Signed-off-by: Albert Teoh <[email protected]>

Cleanup

1ba1aa3

Signed-off-by: Albert Teoh <[email protected]>

Merge branch 'master' into golangci-lint

4b102ba

albertteoh commented Aug 30, 2021

View reviewed changes

.golangci.yml

- G404

- G601

gosimple:

go: "1.17"

Copy link

Contributor Author

albertteoh Aug 30, 2021

Choose a reason for hiding this comment

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

Bumped the go version since we've moved onto 1.17 already.

yurishkuro previously approved these changes Sep 2, 2021

View reviewed changes

albertteoh dismissed yurishkuro’s stale review via cb4e345 September 2, 2021 01:44

Print .import.log on lint error

8b3a050

Signed-off-by: Albert Teoh <[email protected]>

albertteoh force-pushed the golangci-lint branch from cb4e345 to 8b3a050 Compare September 2, 2021 02:07

yurishkuro approved these changes Sep 2, 2021

View reviewed changes

.golangci.yml

Comment on lines +16 to +17

- G404

- G601

Copy link

Member

yurishkuro Sep 2, 2021

Choose a reason for hiding this comment

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

let's fix them in another PR

yurishkuro merged commit 27fe841 into jaegertracing:master Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace old linters with golangci-lint #3237

Replace old linters with golangci-lint #3237

albertteoh commented Aug 29, 2021

codecov bot commented Aug 29, 2021 •

edited

Loading

albertteoh Aug 29, 2021

albertteoh Aug 29, 2021

albertteoh Aug 29, 2021 •

edited

Loading

albertteoh Aug 29, 2021

albertteoh Aug 29, 2021

yurishkuro Sep 2, 2021

albertteoh Aug 29, 2021

albertteoh Aug 29, 2021

yurishkuro Aug 29, 2021

albertteoh Aug 30, 2021

yurishkuro Aug 29, 2021

pavolloffay Aug 30, 2021

albertteoh Aug 30, 2021

albertteoh Aug 30, 2021

albertteoh Aug 30, 2021

albertteoh commented Sep 1, 2021

yurishkuro Sep 2, 2021

albertteoh Sep 2, 2021 •

edited

Loading

yurishkuro Sep 2, 2021

albertteoh commented Sep 2, 2021

		healthCheckHTTPPort = "health-check-http-port"
		adminHTTPPort = "admin-http-port"

Replace old linters with golangci-lint #3237

Replace old linters with golangci-lint #3237

Conversation

albertteoh commented Aug 29, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Aug 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh Aug 29, 2021 • edited Loading

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

albertteoh commented Sep 1, 2021

Choose a reason for hiding this comment

albertteoh Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh commented Sep 2, 2021

codecov bot commented Aug 29, 2021 •

edited

Loading

albertteoh Aug 29, 2021 •

edited

Loading

albertteoh Sep 2, 2021 •

edited

Loading