diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2020-02-06 12:25:43 +0300 |
---|---|---|
committer | Pavlo Strokov <pstrokov@gitlab.com> | 2020-02-06 12:25:43 +0300 |
commit | 8a549719107d4acfe843289ceed388113997c396 (patch) | |
tree | 44ef10c47ba87867bc48bfea0feee0cb7f557a83 | |
parent | be362716796f9283d34f30dfcdc6e62c76da7b0b (diff) | |
parent | 713d1e48494044d8e5b1d630c67b26d19a84cae1 (diff) |
Merge branch 'ps-golangci-static-code-analys' into 'master'
Use golangci-lint for static code analysis
Closes #2253
See merge request gitlab-org/gitaly!1722
-rw-r--r-- | .gitlab-ci.yml | 37 | ||||
-rw-r--r-- | .golangci.yml | 32 | ||||
-rw-r--r-- | Makefile | 8 | ||||
-rw-r--r-- | _support/Makefile.template | 50 | ||||
-rw-r--r-- | _support/golangci.warnings.yml | 20 | ||||
-rw-r--r-- | _support/lint.go | 53 | ||||
-rw-r--r-- | _support/makegen.go | 33 | ||||
-rwxr-xr-x | _support/release | 2 | ||||
-rw-r--r-- | changelogs/unreleased/ps-golangci-static-code-analys.yml | 5 | ||||
-rw-r--r-- | internal/middleware/cache/cache_test.go | 2 | ||||
-rw-r--r-- | internal/praefect/grpc-proxy/proxy/handler_test.go | 2 | ||||
-rw-r--r-- | internal/praefect/grpc-proxy/proxy/helper_test.go | 4 | ||||
-rw-r--r-- | internal/service/operations/rebase_test.go | 2 | ||||
-rw-r--r-- | internal/service/repository/archive_test.go | 2 | ||||
-rw-r--r-- | proto/go/internal/linter/method.go | 8 |
15 files changed, 159 insertions, 101 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d02ca67e1..2d00ce770 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -83,13 +83,6 @@ verify: script: - make verify -verify-warnings: - <<: *ruby_definition - stage: test - script: - - make verify-warnings - allow_failure: true - proto: <<: *ruby_definition stage: test @@ -225,3 +218,33 @@ praefect_sql_connect: - ruby -rerb -e 'ERB.new(ARGF.read).run' _support/config.praefect.toml.ci-sql-test.erb > config.praefect.toml - ./praefect -config config.praefect.toml sql-ping - ./praefect -config config.praefect.toml sql-migrate + +lint: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:golangci-lint-alpine + stage: test + before_script: + # this removes dependency to the github.com/libgit2/git2go: https://gitlab.com/gitlab-org/gitaly/issues/1706 + - git rm -r --quiet cmd/gitaly-remote + script: + # Write the code coverage report to gl-code-quality-report.json + # and print linting issues to stdout in the format: path/to/file:line description + - golangci-lint run --config .golangci.yml --out-format code-climate | tee gl-code-quality-report.json | jq -r '.[] | "\(.location.path):\(.location.lines.begin) \(.description)"' + after_script: + - git checkout --quiet --no-progress HEAD cmd/gitaly-remote # restore removed code + artifacts: + reports: + codequality: gl-code-quality-report.json + paths: + - gl-code-quality-report.json + +lint-warnings: + image: registry.gitlab.com/gitlab-org/gitlab-build-images:golangci-lint-alpine + stage: test + before_script: + # this removes dependency to the github.com/libgit2/git2go: https://gitlab.com/gitlab-org/gitaly/issues/1706 + - git rm -r --quiet cmd/gitaly-remote + script: + - golangci-lint run --config _support/golangci.warnings.yml --out-format tab + after_script: + - git checkout --quiet --no-progress HEAD cmd/gitaly-remote # restore removed code + allow_failure: true diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..f9af40c86 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,32 @@ +# options for analysis running +run: + # timeout for analysis, e.g. 30s, 5m, default is 1m + timeout: 5m + modules-download-mode: readonly + +# all available settings of specific linters +linters-settings: + staticcheck: + checks: + - inherit + +# list of useful linters could be found at https://github.com/golangci/awesome-go-linters +linters: + disable-all: true + enable: + - golint + - goimports # https://godoc.org/golang.org/x/tools/cmd/goimports + - staticcheck # https://github.com/dominikh/go-tools/tree/master/cmd/staticcheck + - gosimple # https://github.com/dominikh/go-tools/tree/master/cmd/gosimple + +issues: + # Excluding configuration per-path, per-linter, per-text and per-source + exclude-rules: + # Exclude some staticcheck messages + - linters: + - staticcheck + text: "SA1019:" + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-issues-per-linter: 0 + # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. + max-same-issues: 0 @@ -80,8 +80,12 @@ rspec-gitlab-shell: prepare-build verify: prepare-build cd $(BUILD_DIR) && $(MAKE) $@ -.PHONY: verify-warnings -verify-warnings: prepare-build +.PHONY: lint +lint: prepare-build + cd $(BUILD_DIR) && $(MAKE) $@ + +.PHONY: lint-warnings +lint-warnings: prepare-build cd $(BUILD_DIR) && $(MAKE) $@ .PHONY: format diff --git a/_support/Makefile.template b/_support/Makefile.template index 80a3b0457..d522ef53c 100644 --- a/_support/Makefile.template +++ b/_support/Makefile.template @@ -150,7 +150,7 @@ rspec-gitlab-shell: {{ .GitlabShellDir }}/config.yml assemble-go prepare-tests @cd {{ .GitalyRubyDir }} && bundle exec bin/ruby-cd {{ .GitlabShellDir }} rspec .PHONY: verify -verify: check-mod-tidy lint check-formatting staticcheck notice-up-to-date check-proto rubocop +verify: check-mod-tidy check-formatting notice-up-to-date check-proto rubocop .PHONY: check-mod-tidy check-mod-tidy: @@ -159,16 +159,15 @@ check-mod-tidy: .PHONY: lint lint: {{ .GoLint }} - # golint - @cd {{ .SourceDir }} && go run _support/lint.go - -{{ .GoLint }}: - go get golang.org/x/lint/golint@959b441ac422379a43da2230f62be024250818b0 + @cd {{ .SourceDir }} && \ + git rm -r --quiet cmd/gitaly-remote; \ + {{ .GoLint }} run --out-format tab --config .golangci.yml; \ + EXIT_CODE=$$?;\ + git checkout --quiet --no-progress HEAD cmd/gitaly-remote; \ + exit $$EXIT_CODE .PHONY: check-formatting -check-formatting: {{ .GoImports }} {{ .GitalyFmt }} - # goimports - @cd {{ .SourceDir }} && goimports -e -l {{ join .GoFiles " " }} | {{ .MakeFormatCheck }} +check-formatting: {{ .GitalyFmt }} # gitalyfmt @cd {{ .SourceDir }} && {{ .GitalyFmt }} {{ join .GoFiles " " }} | {{ .MakeFormatCheck }} @@ -187,24 +186,20 @@ format: {{ .GoImports }} {{ .GitalyFmt }} # goimports pass 2 @cd {{ .SourceDir }} && goimports -w -l {{ join .GoFiles " " }} -.PHONY: staticcheck -staticcheck: {{ .StaticCheck }} - # staticcheck runs all default checks minus deprecations (SA1019) - @cd {{ .SourceDir }} && {{ .StaticCheck }} -checks inherit,-SA1019 -tags "$(BUILD_TAGS) static" {{ join .AllPackages " " }} - .PHONY: staticcheck-deprecations -staticcheck-deprecations: {{ .StaticCheck }} +staticcheck-deprecations: {{ .GoLint }} # Only returns deprecated code usage - @cd {{ .SourceDir }} && {{ .StaticCheck }} -checks SA1019 -tags "$(BUILD_TAGS) static" {{ join .AllPackages " " }} - -.PHONY: verify-warnings -verify-warnings: staticcheck-deprecations + @cd {{ .SourceDir }} && \ + git rm -r --quiet cmd/gitaly-remote; \ + {{ .GoLint }} run --out-format tab --config _support/golangci.warnings.yml; \ + EXIT_CODE=$$?;\ + git checkout --quiet --no-progress HEAD cmd/gitaly-remote; \ + exit $$EXIT_CODE + +.PHONY: lint-warnings +lint-warnings: staticcheck-deprecations # Runs verification analysis that is okay to fail (but not ignore completely) -# Install staticcheck -{{ .StaticCheck }}: - go get honnef.co/go/tools/cmd/staticcheck@95959eaf5e3c41c66151dcfd91779616b84077a8 - {{ .GoVendor }}: go get github.com/kardianos/govendor@e07957427183a9892f35634ffc9ea48dedc6bbb4 @@ -291,6 +286,15 @@ proto: {{ .ProtoC }} {{ .ProtoCGenGo }} {{ .ProtoCGenGitaly }} {{ .GrpcToolsRuby {{ .GrpcToolsRuby }}: gem install --bindir {{ .BuildDir }}/bin -v 1.0.1 grpc-tools +{{ .GoLint }}: {{ .BuildDir }}/golangci-lint.tar.gz + mkdir -p {{ .BuildDir }}/bin + cd {{ .BuildDir }} && tar -x -z --strip-components 1 -C {{ .BuildDir }}/bin -f golangci-lint.tar.gz {{ .GolangCILint }}/golangci-lint + +{{ .BuildDir }}/golangci-lint.tar.gz: + curl -o $@.tmp --silent -L {{ .GolangCILintURL }} + printf '{{ .GolangCILintSHA256 }} $@.tmp' | shasum -a256 -c - + mv $@.tmp $@ + no-changes: # looking for changed files @cd {{ .SourceDir }} && git status --porcelain | awk '{ print } END { if (NR > 0) { exit 1 } }' diff --git a/_support/golangci.warnings.yml b/_support/golangci.warnings.yml new file mode 100644 index 000000000..b2f931567 --- /dev/null +++ b/_support/golangci.warnings.yml @@ -0,0 +1,20 @@ +run: + timeout: 5m + issues-exit-code: 1 + tests: true + skip-dirs-use-default: true + modules-download-mode: readonly + +linters-settings: + staticcheck: + checks: + - inherit + +linters: + disable-all: true + enable: + - staticcheck + +issues: + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/_support/lint.go b/_support/lint.go deleted file mode 100644 index 3f938db64..000000000 --- a/_support/lint.go +++ /dev/null @@ -1,53 +0,0 @@ -package main - -import ( - "bufio" - "fmt" - "os" - "os/exec" - "strings" -) - -func main() { - if err := lint(); err != nil { - fmt.Printf("error: %v\n", err) - os.Exit(1) - } -} - -func lint() (err error) { - cmd := exec.Command("golint", "./...") - cmd.Stderr = os.Stderr - stdout, err := cmd.StdoutPipe() - if err != nil { - return nil - } - if err := cmd.Start(); err != nil { - return err - } - defer func() { - if waitErr := cmd.Wait(); waitErr != nil { - err = fmt.Errorf("wait error: %v", waitErr) - } - }() - - scanner := bufio.NewScanner(stdout) - offenses := 0 - for scanner.Scan() { - line := scanner.Text() - if strings.HasPrefix(line, "vendor/") || strings.HasPrefix(line, "ruby/vendor/") { - // We cannot and should not "fix" vendored code. - continue - } - offenses++ - fmt.Println(line) - } - if err := scanner.Err(); err != nil { - return fmt.Errorf("scanner error: %v", err) - } - - if offenses > 0 { - return fmt.Errorf("golint found %d offense(s)", offenses) - } - return nil -} diff --git a/_support/makegen.go b/_support/makegen.go index 6845d76d2..ab836951c 100644 --- a/_support/makegen.go +++ b/_support/makegen.go @@ -74,7 +74,7 @@ func (gm *gitalyMake) Pkg() string { return "gitlab.com/gitlab-org/gital func (gm *gitalyMake) GoImports() string { return "bin/goimports" } func (gm *gitalyMake) GitalyFmt() string { return filepath.Join(gm.BuildDir(), "bin/gitalyfmt") } func (gm *gitalyMake) GoCovMerge() string { return "bin/gocovmerge" } -func (gm *gitalyMake) GoLint() string { return "bin/golint" } +func (gm *gitalyMake) GoLint() string { return filepath.Join(gm.BuildDir(), "bin/golangci-lint") } func (gm *gitalyMake) GoVendor() string { return "bin/govendor" } func (gm *gitalyMake) StaticCheck() string { return filepath.Join(gm.BuildDir(), "bin/staticcheck") } func (gm *gitalyMake) ProtoC() string { return filepath.Join(gm.BuildDir(), "protoc/bin/protoc") } @@ -281,17 +281,17 @@ func (gm *gitalyMake) AllPackages() []string { return pkgs } -type protoDownloadInfo struct { +type downloadInfo struct { url string sha256 string } -var protoCDownload = map[string]protoDownloadInfo{ - "darwin/amd64": protoDownloadInfo{ +var protoCDownload = map[string]downloadInfo{ + "darwin/amd64": downloadInfo{ url: "https://github.com/protocolbuffers/protobuf/releases/download/v3.6.1/protoc-3.6.1-osx-x86_64.zip", sha256: "0decc6ce5beed07f8c20361ddeb5ac7666f09cf34572cca530e16814093f9c0c", }, - "linux/amd64": protoDownloadInfo{ + "linux/amd64": downloadInfo{ url: "https://github.com/protocolbuffers/protobuf/releases/download/v3.6.1/protoc-3.6.1-linux-x86_64.zip", sha256: "6003de742ea3fcf703cfec1cd4a3380fd143081a2eb0e559065563496af27807", }, @@ -309,6 +309,29 @@ func (gm *gitalyMake) MakeFormatCheck() string { return `awk '{ print } END { if(NR>0) { print "Formatting error, run make format"; exit(1) } }'` } +func (gm *gitalyMake) GolangCILintURL() string { + return "https://github.com/golangci/golangci-lint/releases/download/v" + gm.GolangCILintVersion() + "/" + gm.GolangCILint() + ".tar.gz" +} + +func (gm *gitalyMake) GolangCILintSHA256() string { + switch runtime.GOOS + "/" + runtime.GOARCH { + case "darwin/amd64": + return "fcf80824c21567eb0871055711bf9bdca91cf9a081122e2a45f1d11fed754600" + case "linux/amd64": + return "109d38cdc89f271392f5a138d6782657157f9f496fd4801956efa2d0428e0cbe" + default: + return "unknown" + } +} + +func (gm *gitalyMake) GolangCILintVersion() string { + return "1.22.2" +} + +func (gm *gitalyMake) GolangCILint() string { + return fmt.Sprintf("golangci-lint-%s-%s-%s", gm.GolangCILintVersion(), runtime.GOOS, runtime.GOARCH) +} + var templateText = func() string { contents, err := ioutil.ReadFile("../_support/Makefile.template") if err != nil { diff --git a/_support/release b/_support/release index 00a592bdb..709805e8a 100755 --- a/_support/release +++ b/_support/release @@ -10,7 +10,7 @@ def main(version) end env = %w[/usr/bin/env BUNDLE_FLAGS=--no-deployment] - run!(env + %w[make verify smoke-test]) + run!(env + %w[make verify lint smoke-test]) puts 'Testing for changed files' run!(%w[git diff --quiet --exit-code]) diff --git a/changelogs/unreleased/ps-golangci-static-code-analys.yml b/changelogs/unreleased/ps-golangci-static-code-analys.yml new file mode 100644 index 000000000..9ba1498ac --- /dev/null +++ b/changelogs/unreleased/ps-golangci-static-code-analys.yml @@ -0,0 +1,5 @@ +--- +title: Use golangci-lint for static code analysis +merge_request: 1722 +author: +type: other diff --git a/internal/middleware/cache/cache_test.go b/internal/middleware/cache/cache_test.go index 0d9d126de..d01087235 100644 --- a/internal/middleware/cache/cache_test.go +++ b/internal/middleware/cache/cache_test.go @@ -168,7 +168,7 @@ func streamFileDesc(t testing.TB) *descriptor.FileDescriptorProto { return fdp } -func newTestSvc(t testing.TB, ctx context.Context, srvr *grpc.Server, svc testdata.TestServiceServer) (testdata.TestServiceClient, *grpc.ClientConn, func()) { +func newTestSvc(t testing.TB, ctx context.Context, srvr *grpc.Server, svc testdata.TestServiceServer) (testdata.TestServiceClient, *grpc.ClientConn, func()) { //nolint:golint healthSrvr := health.NewServer() grpc_health_v1.RegisterHealthServer(srvr, healthSrvr) healthSrvr.SetServingStatus("TestService", grpc_health_v1.HealthCheckResponse_SERVING) diff --git a/internal/praefect/grpc-proxy/proxy/handler_test.go b/internal/praefect/grpc-proxy/proxy/handler_test.go index 629d153c9..bbd89886d 100644 --- a/internal/praefect/grpc-proxy/proxy/handler_test.go +++ b/internal/praefect/grpc-proxy/proxy/handler_test.go @@ -90,7 +90,7 @@ func (s *assertingService) PingStream(stream pb.TestService_PingStreamServer) er if err := stream.Send(pong); err != nil { require.NoError(s.t, err, "can't fail sending back a pong") } - counter += 1 + counter++ } stream.SetTrailer(metadata.Pairs(serverTrailerMdKey, "I like ending turtles.")) return nil diff --git a/internal/praefect/grpc-proxy/proxy/helper_test.go b/internal/praefect/grpc-proxy/proxy/helper_test.go index 64fea0493..c56355638 100644 --- a/internal/praefect/grpc-proxy/proxy/helper_test.go +++ b/internal/praefect/grpc-proxy/proxy/helper_test.go @@ -18,7 +18,7 @@ func newListener(tb testing.TB) net.Listener { return listener } -func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *interceptPinger, func()) { +func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *interceptPinger, func()) { //nolint:golint ip := &interceptPinger{} done := make(chan struct{}) @@ -52,7 +52,7 @@ func newBackendPinger(tb testing.TB, ctx context.Context) (*grpc.ClientConn, *in return cc, ip, cleanup } -func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector, svc, method string) (*grpc.ClientConn, func()) { +func newProxy(tb testing.TB, ctx context.Context, director proxy.StreamDirector, svc, method string) (*grpc.ClientConn, func()) { //nolint:golint proxySrvr := grpc.NewServer( grpc.CustomCodec(proxy.Codec()), grpc.UnknownServiceHandler(proxy.TransparentHandler(director)), diff --git a/internal/service/operations/rebase_test.go b/internal/service/operations/rebase_test.go index c064b02c1..af6e25270 100644 --- a/internal/service/operations/rebase_test.go +++ b/internal/service/operations/rebase_test.go @@ -677,7 +677,7 @@ func recvTimeout(bidi gitalypb.OperationService_UserRebaseConfirmableClient, tim } } -func buildHeaderRequest(repo *gitalypb.Repository, user *gitalypb.User, rebaseId string, branchName string, branchSha string, remoteRepo *gitalypb.Repository, remoteBranch string) *gitalypb.UserRebaseConfirmableRequest { +func buildHeaderRequest(repo *gitalypb.Repository, user *gitalypb.User, rebaseId string, branchName string, branchSha string, remoteRepo *gitalypb.Repository, remoteBranch string) *gitalypb.UserRebaseConfirmableRequest { // nolint:golint return &gitalypb.UserRebaseConfirmableRequest{ UserRebaseConfirmableRequestPayload: &gitalypb.UserRebaseConfirmableRequest_Header_{ &gitalypb.UserRebaseConfirmableRequest_Header{ diff --git a/internal/service/repository/archive_test.go b/internal/service/repository/archive_test.go index 0743c484d..5e129b8f9 100644 --- a/internal/service/repository/archive_test.go +++ b/internal/service/repository/archive_test.go @@ -271,7 +271,7 @@ func TestGetArchivePathInjection(t *testing.T) { ssh-ed25519 my_super_evil_ssh_pubkey #`) - _, err = fmt.Fprintf(f, evilPubKeyFile) + _, err = fmt.Fprint(f, evilPubKeyFile) require.NoError(t, err) require.NoError(t, f.Close()) diff --git a/proto/go/internal/linter/method.go b/proto/go/internal/linter/method.go index bccdeffd5..e7334a717 100644 --- a/proto/go/internal/linter/method.go +++ b/proto/go/internal/linter/method.go @@ -90,7 +90,7 @@ func (ml methodLinter) ensureValidStorage(expected int) error { msgT := topLevelMsgs[reqMsgName] - storageFields, err := findStorageFields(topLevelMsgs, reqMsgName, msgT) + storageFields, err := findStorageFields(topLevelMsgs, reqMsgName, msgT) // nolint:staticcheck if len(storageFields) != expected { return fmt.Errorf("unexpected count of storage field %d, expected %d, found storage label at: %v", len(storageFields), expected, storageFields) @@ -107,7 +107,7 @@ func findStorageFields(topLevelMsgs map[string]*descriptor.DescriptorProto, pref return nil, err } if storage { - storageFields = append(storageFields, prefix + "." + f.GetName()) + storageFields = append(storageFields, prefix+"."+f.GetName()) } childMsg, err := findChildMsg(topLevelMsgs, t, f) @@ -116,7 +116,7 @@ func findStorageFields(topLevelMsgs map[string]*descriptor.DescriptorProto, pref } if childMsg != nil { - nestedStorageFields, err := findStorageFields(topLevelMsgs, prefix + "." + f.GetName(), childMsg) + nestedStorageFields, err := findStorageFields(topLevelMsgs, prefix+"."+f.GetName(), childMsg) if err != nil { return nil, err } @@ -136,7 +136,7 @@ func findChildMsg(topLevelMsgs map[string]*descriptor.DescriptorProto, t *descri msgName, err := lastName(f.GetTypeName()) if err != nil { - return nil, err + return nil, err } for _, nestedType := range t.GetNestedType() { |