Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace old linters with golangci-lint #3237

Merged
merged 12 commits into from
Sep 2, 2021
Merged

Replace old linters with golangci-lint #3237

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1e67b6d
Add golangci-lint support
albertteoh Aug 24, 2021
f591546
Merge branch 'master' into golangci-lint
albertteoh Aug 29, 2021
ceda4be
Redundant
albertteoh Aug 29, 2021
c5b8e2d
Remove whitespace
albertteoh Aug 29, 2021
5fb5d53
Move golangci-lint into unit test workflow
albertteoh Aug 29, 2021
7b1ab1c
Move golanci-lint step into Unit test job
albertteoh Aug 29, 2021
39a4051
Address review comments
albertteoh Aug 30, 2021
3c23f85
Newline
albertteoh Aug 30, 2021
979b30b
fmt -> lic
albertteoh Aug 30, 2021
1ba1aa3
Cleanup
albertteoh Aug 30, 2021
4b102ba
Merge branch 'master' into golangci-lint
albertteoh Aug 30, 2021
8b3a050
Print .import.log on lint error
albertteoh Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/ci-golangci-lint.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
name: Run golangci-lint
on:
push:
branches: [ master ]
pull_request:
branches: [ master ]
jobs:
golangci:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: golangci-lint
uses: golangci/golangci-lint-action@v2
with:
# Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
version: v1.42
36 changes: 36 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
run:
timeout: 10m
skip-dirs:
- swagger-gen
- pkg/cassandra/mocks
- storage/mocks
- cmd/ingester/app/consumer/mocks

linters-settings:
gosec:
# To specify a set of rules to explicitly exclude.
# Available rules: https://github.com/securego/gosec#available-rules
excludes:
- G104
- G107
Comment on lines +14 to +15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

- G404
- G601
Comment on lines +16 to +17
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

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

gosimple:
go: "1.16"

linters:
enable:
- gofmt
- goimports
- gosec
- govet
disable:
- errcheck

issues:
# Excluding configuration per-path, per-linter, per-text and per-source
exclude-rules:
# Exclude some linters from running on tests files.
- path: _test\.go
linters:
- gosec
49 changes: 5 additions & 44 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ ALL_SRC := $(shell find . -name '*.go' \
-type f | \
sort)

# ALL_PKGS is used with 'golint'
# ALL_PKGS is used with 'nocover'
ALL_PKGS := $(shell echo $(dir $(ALL_SRC)) | tr ' ' '\n' | sort -u)

UNAME := $(shell uname -m)
Expand All @@ -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
Copy link
Contributor Author

@albertteoh albertteoh Aug 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

GOVET=go vet
GOFMT=gofmt
FMT_LOG=.fmt.log
LINT_LOG=.lint.log
IMPORT_LOG=.import.log

GIT_SHA=$(shell git rev-parse HEAD)
GIT_CLOSEST_TAG=$(shell git describe --abbrev=0 --tags)
Expand Down Expand Up @@ -82,7 +77,7 @@ go-gen:

.PHONY: clean
clean:
rm -rf cover.out .cover/ cover.html lint.log fmt.log \
rm -rf cover.out .cover/ cover.html \
jaeger-ui/packages/jaeger-ui/build

.PHONY: test
Expand Down Expand Up @@ -130,7 +125,6 @@ index-rollover-integration-test: docker-images-elastic
go clean -testcache
bash -c "set -e; set -o pipefail; $(GOTEST) -tags index_rollover $(STORAGE_PKGS) | $(COLORIZE)"


.PHONY: token-propagation-integration-test
token-propagation-integration-test:
go clean -testcache
Expand Down Expand Up @@ -161,39 +155,9 @@ fmt:
@$(GOFMT) -e -s -l -w $(ALL_SRC)
./scripts/updateLicenses.sh

.PHONY: lint-gosec
lint-gosec:
time gosec -quiet -exclude=G104,G107 ./...

.PHONY: lint-staticcheck
lint-staticcheck:
@cat /dev/null > $(LINT_LOG)
time staticcheck ./... \
| grep -v \
-e model/model.pb.go \
-e proto-gen \
-e _test.pb.go \
-e thrift-gen/ \
-e swagger-gen/ \
>> $(LINT_LOG) || true
@[ ! -s "$(LINT_LOG)" ] || (echo "Detected staticcheck failures:" | cat - $(LINT_LOG) && false)

.PHONY: lint
lint: lint-staticcheck lint-gosec
$(GOVET) ./...
$(MAKE) go-lint
@echo Running go fmt on ALL_SRC ...
@$(GOFMT) -e -s -l $(ALL_SRC) > $(FMT_LOG)
./scripts/updateLicenses.sh >> $(FMT_LOG)
Copy link
Member

Choose a reason for hiding this comment

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

This must remain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

./scripts/import-order-cleanup.sh stdout > $(IMPORT_LOG)
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
@[ ! -s "$(FMT_LOG)" -a ! -s "$(IMPORT_LOG)" ] || (echo "Go fmt, license check, or import ordering failures, run 'make fmt'" | cat - $(FMT_LOG) && false)

.PHONY: go-lint
go-lint:
@cat /dev/null > $(LINT_LOG)
@echo Running go lint...
@$(GOLINT) $(ALL_PKGS) | grep -v _nolint.go >> $(LINT_LOG) || true;
@[ ! -s "$(LINT_LOG)" ] || (echo "Lint Failures" | cat - $(LINT_LOG) && false)
lint:
golangci-lint run

.PHONY: build-examples
build-examples:
Expand Down Expand Up @@ -400,16 +364,13 @@ changelog:
.PHONY: install-tools
install-tools:
go install github.com/wadey/gocovmerge
go install golang.org/x/lint/golint
go install github.com/mjibson/esc
go install github.com/securego/gosec/cmd/gosec
go install honnef.co/go/tools/cmd/staticcheck

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

.PHONY: test-ci
test-ci: build-examples lint cover
test-ci: build-examples cover
Copy link
Member

Choose a reason for hiding this comment

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

I would consider killing this target and invoking directly in GA

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


.PHONY: thrift
thrift: idl/thrift/jaeger.thrift thrift-image
Expand Down
4 changes: 2 additions & 2 deletions cmd/agent/app/reporter/grpc/reporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestReporter_EmitZipkinBatch(t *testing.T) {
})
defer s.Stop()
conn, err := grpc.Dial(addr.String(), grpc.WithInsecure())
//lint:ignore SA5001 don't care about errors
//nolint:staticcheck // don't care about errors
defer conn.Close()
require.NoError(t, err)

Expand Down Expand Up @@ -94,7 +94,7 @@ func TestReporter_EmitBatch(t *testing.T) {
})
defer s.Stop()
conn, err := grpc.Dial(addr.String(), grpc.WithInsecure())
//lint:ignore SA5001 don't care about errors
//nolint:staticcheck // don't care about errors
defer conn.Close()
require.NoError(t, err)
rep := NewReporter(conn, nil, zap.NewNop())
Expand Down
4 changes: 1 addition & 3 deletions cmd/flags/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ import (
)

const (
healthCheckHTTPPort = "health-check-http-port"
adminHTTPPort = "admin-http-port"
Comment on lines -35 to -36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused and deprecated here.

adminHTTPHostPort = "admin.http.host-port"
adminHTTPHostPort = "admin.http.host-port"
)

// AdminServer runs an HTTP server with admin endpoints, such as healthcheck at /, /metrics, etc.
Expand Down
2 changes: 1 addition & 1 deletion pkg/gogocodec/codec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ package gogocodec
import (
"testing"

"github.com/golang/protobuf/proto" //lint:ignore SA1019 deprecated package
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"google.golang.org/protobuf/types/known/emptypb"

"github.com/jaegertracing/jaeger/model"
Expand Down
2 changes: 1 addition & 1 deletion pkg/queue/bounded_queue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func helper(t *testing.T, startConsumers func(q *BoundedQueue, consumerFn func(i

// block further processing until startLock is released
startLock.Lock()
//lint:ignore SA2001 empty section is ok
//nolint:staticcheck // empty section is ok
startLock.Unlock()
})

Expand Down