Use golangci-lint for static code analysis

New job 'lint' added to 'test' stage to perform static code analysis
using a common approach with golangci-lint tool described at
https://docs.gitlab.com/ee/development/go_guide/#automatic-linting

Closes: https://gitlab.com/gitlab-org/gitaly/issues/2253

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
diff --git a/Makefile b/Makefile
index 4a49729c2..a0cfa73ad 100644
--- a/Makefile
+++ b/Makefile
@@ -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 b4aa56307..5eb94604a 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 a5afbc1f1..ab1700798 100644
--- a/internal/service/repository/archive_test.go
+++ b/internal/service/repository/archive_test.go
@@ -269,7 +269,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() {