VAULT-17734, VAULT-17735: Combine linters (#21611)

* combine into one checker * combine and simplify ci checks * add to test package list * remove testing test * only run deprecations check * only run deprecations check * remove unneeded repo check * fix bash options
2025-10-29 09:42:25 +00:00 · 2023-07-06 15:18:42 +02:00
parent 02f43ecbc2
commit 8c18f24b9d
19 changed files with 122 additions and 198 deletions
--- a/.github/scripts/generate-test-package-lists.sh
+++ b/.github/scripts/generate-test-package-lists.sh
@@ -45,8 +45,8 @@ fi
 test_packages[4]+=" $base/http"
 test_packages[4]+=" $base/sdk/helper/pluginutil"
 test_packages[4]+=" $base/serviceregistration/kubernetes"
-test_packages[4]+=" $base/tools/godoctests/pkg/analyzer"
-test_packages[4]+=" $base/tools/gonilnilfunctions/pkg/analyzer"
+test_packages[4]+=" $base/tools/codechecker/pkg/godoctests"
+test_packages[4]+=" $base/tools/codechecker/pkg/gonilnilfunctions"
 if [ "${ENTERPRISE:+x}" == "x" ] ; then
    test_packages[4]+=" $base/vault/external_tests/apilock"
    test_packages[4]+=" $base/vault/external_tests/filteredpaths"
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -48,18 +48,6 @@ jobs:
            echo 'enterprise=' >> "$GITHUB_OUTPUT"
            echo 'go-build-tags=' >> "$GITHUB_OUTPUT"
          fi
-  semgrep:
-    name: Semgrep
-    needs:
-      - setup
-    runs-on: ${{ fromJSON(needs.setup.outputs.compute-tiny) }}
-    container:
-      image: returntocorp/semgrep@sha256:ffc6f3567654f9431456d49fd059dfe548f007c494a7eb6cd5a1a3e50d813fb3
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-      - name: Run Semgrep Rules
-        id: semgrep
-        run: semgrep ci --include '*.go' --config 'tools/semgrep/ci'
  setup-go-cache:
    name: Go Caches
    needs:
@@ -68,25 +56,6 @@ jobs:
    with:
      runs-on: ${{ needs.setup.outputs.compute-standard }}
    secrets: inherit
-  fmt:
-    name: Check Format
-    needs:
-      - setup
-    runs-on: ${{ fromJSON(needs.setup.outputs.compute-tiny) }}
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          go-version-file: ./.go-version
-          cache: true
-      - id: format
-        run: |
-          echo "Using gofumpt version $(go run mvdan.cc/gofumpt -version)"
-          make fmt
-          if ! git diff --exit-code; then
-            echo "Code has formatting errors. Run 'make fmt' to fix"
-            exit 1
-          fi
  diff-oss-ci:
    name: Diff OSS
    needs:
--- a/.github/workflows/code-checker.yml
+++ b/.github/workflows/code-checker.yml
@@ -0,0 +1,73 @@
+name: Run linters
+
+on:
+  pull_request:
+    types: [opened, synchronize, reopened, ready_for_review]
+  push:
+    branches:
+      - main
+      - release/**
+concurrency:
+  group: ${{ github.head_ref || github.run_id }}-lint
+  cancel-in-progress: true
+
+jobs:
+  deprecations:
+    name: Deprecated functions
+    runs-on: ubuntu-latest
+    if: github.base_ref == 'main'
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+        with:
+          fetch-depth: 0
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+      - run: make ci-deprecations
+        name: Check deprecations
+  codechecker:
+    name: Code checks 
+    runs-on: ubuntu-latest
+    if: github.base_ref == 'main'
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+        with:
+          fetch-depth: 0
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+        # Note: if there is a function we want to ignore the nilnil check for,
+        # You can add 'ignore-nil-nil-function-check' somewhere in the
+        # godoc for the function.
+      - run: make ci-vet-codechecker
+        name: Check custom linters
+  format:
+    name: Format
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+      - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
+        with:
+          go-version-file: ./.go-version
+          cache: true
+      - name: Go format
+        run: |
+          make ci-bootstrap
+          echo "Using gofumpt version $(go run mvdan.cc/gofumpt -version)"
+          make fmt
+          if ! git diff --exit-code; then
+            echo "Code has formatting errors. Run 'make fmt' to fix"
+            exit 1
+          fi
+  semgrep:
+    name: Semgrep
+    runs-on: ubuntu-latest
+    container:
+      image: returntocorp/semgrep@sha256:ffc6f3567654f9431456d49fd059dfe548f007c494a7eb6cd5a1a3e50d813fb3
+    steps:
+      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+      - name: Run Semgrep Rules
+        id: semgrep
+        run: semgrep ci --include '*.go' --config 'tools/semgrep/ci'
--- a/.github/workflows/drepecated-functions-checker.yml
+++ b/.github/workflows/drepecated-functions-checker.yml
@@ -1,31 +0,0 @@
-name: "Check Deprecations"
-
-on:
-  pull_request:
-    # Runs on PRs to main
-    branches:
-      - main
-
-jobs:
-  deprecations-check:
-    runs-on: ubuntu-latest
-    timeout-minutes: 30
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-        with:
-          fetch-depth: 0 # by default the checkout action doesn't checkout all branches
-      - name: Setup Go
-        uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          go-version-file: ./.go-version
-          cache: true
-      - name: Install required tools
-        run: |
-          make bootstrap
-      - name: Check deprecations for files in diff
-        run: |
-          # Need to run this from repository root and not from scripts/ as staticcheck works 
-          # only on packages
-          ./scripts/deprecations-checker.sh ${{ github.event.pull_request.base.ref }} ${{ github.event.repository.name }}
-          
--- a/.github/workflows/godoc-test-checker.yml
+++ b/.github/workflows/godoc-test-checker.yml
@@ -1,23 +0,0 @@
-name: Check Go Docs for tests
-
-on:
-  pull_request:
-    types: [opened, synchronize]
-    # Runs on PRs to main
-    branches:
-      - main
-
-jobs:
-  godoc-test-check:
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-        with:
-          fetch-depth: 0
-      - name: Set Up Go
-        uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          cache: true
-          go-version-file: ./.go-version
-      - name: Verify new tests have go docs
-        run: make ci-vet-godoctests
--- a/.github/workflows/nil-nil-function-checker.yml
+++ b/.github/workflows/nil-nil-function-checker.yml
@@ -1,26 +0,0 @@
-name: Check Functions For nil, nil returns
-
-on:
-  pull_request:
-    types: [opened, synchronize]
-    # Runs on PRs to main
-    branches:
-      - main
-
-jobs:
-  # Note: if there is a function we want to ignore this check for,
-  # You can add 'ignore-nil-nil-function-check' somewhere in the
-  # godoc for the function.
-  nil-nil-function-check:
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
-        with:
-          fetch-depth: 0
-      - name: Set Up Go
-        uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1
-        with:
-          cache: true
-          go-version-file: ./.go-version
-      - name: Verify functions don't return nil, nil
-        run: make ci-vet-gonilnilfunctions
--- a/.gitignore
+++ b/.gitignore
@@ -129,3 +129,5 @@ website/components/node_modules

 tools/godoctests/.bin
 tools/gonilnilfunctions/.bin
+tools/codechecker/.bin
+.ci-bootstrap
--- a/62
+++ b/62
@@ -9,7 +9,9 @@ INTEG_TEST_TIMEOUT=120m
 VETARGS?=-asmdecl -atomic -bool -buildtags -copylocks -methods -nilfunc -printf -rangeloops -shift -structtags -unsafeptr
 EXTERNAL_TOOLS_CI=\
 	golang.org/x/tools/cmd/goimports \
-	github.com/golangci/revgrep/cmd/revgrep
+	github.com/golangci/revgrep/cmd/revgrep \
+	mvdan.cc/gofumpt \
+	honnef.co/go/tools/cmd/staticcheck
 EXTERNAL_TOOLS=\
 	github.com/client9/misspell/cmd/misspell
 GOFMT_FILES?=$$(find . -name '*.go' | grep -v pb.go | grep -v vendor)
@@ -113,46 +115,28 @@ vet:

 # deprecations runs staticcheck tool to look for deprecations. Checks entire code to see if it 
 # has deprecated function, variable, constant or field
-deprecations:
-	make bootstrap
-	repositoryName=$(basename `git rev-parse --show-toplevel`)
-	./scripts/deprecations-checker.sh "" repositoryName
+deprecations: bootstrap
+	@BUILD_TAGS='$(BUILD_TAGS)' ./scripts/deprecations-checker.sh ""

 # ci-deprecations runs staticcheck tool to look for deprecations. All output gets piped to revgrep
 # which will only return an error if changes that is not on main has deprecated function, variable, constant or field
-ci-deprecations:
-	make bootstrap
-	repositoryName=$(basename `git rev-parse --show-toplevel`)
-	./scripts/deprecations-checker.sh main repositoryName
+ci-deprecations: ci-bootstrap
+	@BUILD_TAGS='$(BUILD_TAGS)' ./scripts/deprecations-checker.sh main

-# tools/godoctests/.bin/godoctests builds the custom analyzer to check for godocs for tests
-tools/godoctests/.bin/godoctests:
-	@cd tools/godoctests && $(GO_CMD) build -o .bin/godoctests .
+tools/codechecker/.bin/codechecker:
+	@cd tools/codechecker && $(GO_CMD) build -o .bin/codechecker .

-# vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep
-# which will only return an error if a new function is missing a godoc
-vet-godoctests: bootstrap tools/godoctests/.bin/godoctests
-	@$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep
-
-# ci-vet-godoctests runs godoctests on the test functions. All output gets piped to revgrep
-# which will only return an error if a new function that is not on main is missing a godoc
-ci-vet-godoctests: ci-bootstrap tools/godoctests/.bin/godoctests
-	@$(GO_CMD) vet -vettool=./tools/godoctests/.bin/godoctests $(TEST) 2>&1 | revgrep origin/main
-
-# tools/gonilnilfunctions/.bin/gonilnilfunctions builds the custom analyzer to check for nil, nil function returns
-tools/gonilnilfunctions/.bin/gonilnilfunctions:
-	@cd tools/gonilnilfunctions && $(GO_CMD) build -o .bin/gonilnilfunctions .
-
-# vet-gonilnilfunctions runs gonilnilfunctions on functions. All output gets piped to revgrep
-# which will only return an error if a new function returns nil, nil (where one of the nils could be an error)
-vet-gonilnilfunctions: bootstrap tools/gonilnilfunctions/.bin/gonilnilfunctions
-	@$(GO_CMD) vet -vettool=./tools/gonilnilfunctions/.bin/gonilnilfunctions ./... 2>&1 | revgrep
-
-# ci-vet-gonilnilfunctions runs gonilnilfunctions on functions. All output gets piped to revgrep
-# which will only return an error if a new function that is not on main has an issue
-ci-vet-gonilnilfunctions: ci-bootstrap tools/gonilnilfunctions/.bin/gonilnilfunctions
-	@$(GO_CMD) vet -vettool=./tools/gonilnilfunctions/.bin/gonilnilfunctions ./... 2>&1 | revgrep origin/main
+# vet-codechecker runs our custom linters on the test functions. All output gets
+# piped to revgrep which will only return an error if new piece of code violates
+# the check 
+vet-codechecker: bootstrap tools/codechecker/.bin/codechecker
+	@$(GO_CMD) vet -vettool=./tools/codechecker/.bin/codechecker -tags=$(BUILD_TAGS) ./... 2>&1 | revgrep

+# vet-codechecker runs our custom linters on the test functions. All output gets
+# piped to revgrep which will only return an error if new piece of code that is 
+# not on main violates the check 
+ci-vet-codechecker: ci-bootstrap tools/codechecker/.bin/codechecker
+	@$(GO_CMD) vet -vettool=./tools/codechecker/.bin/codechecker -tags=$(BUILD_TAGS) ./... 2>&1 | revgrep origin/main

 # lint runs vet plus a number of other checkers, it is more comprehensive, but louder
 lint:
@@ -174,11 +158,13 @@ prep: fmtcheck
 	@if [ -d .git/hooks ]; then cp .hooks/* .git/hooks/; fi

 # bootstrap the build by downloading additional tools needed to build
-ci-bootstrap:
+ci-bootstrap: .ci-bootstrap
+.ci-bootstrap:
 	@for tool in  $(EXTERNAL_TOOLS_CI) ; do \
 		echo "Installing/Updating $$tool" ; \
 		GO111MODULE=off $(GO_CMD) get -u $$tool; \
 	done
+	@touch .ci-bootstrap

 # bootstrap the build by downloading additional tools that may be used by devs
 bootstrap: ci-bootstrap
@@ -255,7 +241,7 @@ fmtcheck:
 	@true
 #@sh -c "'$(CURDIR)/scripts/gofmtcheck.sh'"

-fmt:
+fmt: ci-bootstrap
 	find . -name '*.go' | grep -v pb.go | grep -v vendor | xargs go run mvdan.cc/gofumpt -w

 semgrep:
@@ -296,7 +282,7 @@ hana-database-plugin:
 mongodb-database-plugin:
 	@CGO_ENABLED=0 $(GO_CMD) build -o bin/mongodb-database-plugin ./plugins/database/mongodb/mongodb-database-plugin

-.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci vet-godoctests ci-vet-godoctests vet-gonilnilfunctions ci-vet-gonilnilfunctions
+.PHONY: bin default prep test vet bootstrap ci-bootstrap fmt fmtcheck mysql-database-plugin mysql-legacy-database-plugin cassandra-database-plugin influxdb-database-plugin postgresql-database-plugin mssql-database-plugin hana-database-plugin mongodb-database-plugin ember-dist ember-dist-dev static-dist static-dist-dev assetcheck check-vault-in-path packages build build-ci semgrep semgrep-ci vet-codechecker ci-vet-codechecker 

 .NOTPARALLEL: ember-dist ember-dist-dev

--- a/scripts/deprecations-checker.sh
+++ b/scripts/deprecations-checker.sh
@@ -22,31 +22,17 @@
 # Here, it is used to check if a deprecated function, variable, constant or field is used.

 # Run staticcheck 
+set -e
 echo "Performing deprecations check: running staticcheck"

-# Identify repository name
-if [ -z $2 ]; then
-    # local repository name
-    repositoryName=$(basename `git rev-parse --show-toplevel`)
-else
-    # github repository name from deprecated-functions-checker.yml
-    repositoryName=$2
-fi
-
-# Modify the command with the correct build tag based on repository
-if [ $repositoryName == "vault-enterprise" ]; then
-    staticcheckCommand=$(echo "staticcheck ./... -tags=enterprise")
-else
-    staticcheckCommand=$(echo "staticcheck ./...")
-fi

 # If no compare branch name is specified, output all deprecations
 # Else only output the deprecations from the changes added
 if [ -z $1 ]
    then
-        $staticcheckCommand | grep deprecated
+        staticcheck -checks="SA1019" -tags="$BUILD_TAGS"
    else
        # GitHub Actions will use this to find only changes wrt PR's base ref branch
        # revgrep CLI tool will return an exit status of 1 if any issues match, else it will return 0
-        $staticcheckCommand | grep deprecated 2>&1 | revgrep "$(git merge-base HEAD "origin/$1")"
+        staticcheck  -checks="SA1019" -tags="$BUILD_TAGS" 2>&1 | revgrep "$(git merge-base HEAD "origin/$1")"
 fi
--- a/tools/codechecker/.bin/codechecker
+++ b/tools/codechecker/.bin/codechecker
--- a/tools/codechecker/main.go
+++ b/tools/codechecker/main.go
@@ -0,0 +1,14 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package main
+
+import (
+	"github.com/hashicorp/vault/tools/codechecker/pkg/godoctests"
+	"github.com/hashicorp/vault/tools/codechecker/pkg/gonilnilfunctions"
+	"golang.org/x/tools/go/analysis/multichecker"
+)
+
+func main() {
+	multichecker.Main(gonilnilfunctions.Analyzer, godoctests.Analyzer)
+}
--- a/tools/codechecker/pkg/godoctests/analyzer.go
+++ b/tools/codechecker/pkg/godoctests/analyzer.go
@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0

-package analyzer
+package godoctests

 import (
 	"go/ast"
--- a/tools/gonilnilfunctions/pkg/analyzer/analyzer_test.go
+++ b/tools/gonilnilfunctions/pkg/analyzer/analyzer_test.go
@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0

-package analyzer
+package godoctests

 import (
 	"os"
--- a/tools/codechecker/pkg/godoctests/testdata/funcs.go
+++ b/tools/codechecker/pkg/godoctests/testdata/funcs.go
--- a/tools/codechecker/pkg/gonilnilfunctions/analyzer.go
+++ b/tools/codechecker/pkg/gonilnilfunctions/analyzer.go
@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0

-package analyzer
+package gonilnilfunctions

 import (
 	"go/ast"
--- a/tools/codechecker/pkg/gonilnilfunctions/analyzer_test.go
+++ b/tools/codechecker/pkg/gonilnilfunctions/analyzer_test.go
@@ -1,7 +1,7 @@
 // Copyright (c) HashiCorp, Inc.
 // SPDX-License-Identifier: MPL-2.0

-package analyzer
+package gonilnilfunctions

 import (
 	"os"
--- a/tools/codechecker/pkg/gonilnilfunctions/testdata/funcs.go
+++ b/tools/codechecker/pkg/gonilnilfunctions/testdata/funcs.go
--- a/tools/godoctests/main.go
+++ b/tools/godoctests/main.go
@@ -1,13 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-
-package main
-
-import (
-	"github.com/hashicorp/vault/tools/godoctests/pkg/analyzer"
-	"golang.org/x/tools/go/analysis/singlechecker"
-)
-
-func main() {
-	singlechecker.Main(analyzer.Analyzer)
-}
--- a/tools/gonilnilfunctions/main.go
+++ b/tools/gonilnilfunctions/main.go
@@ -1,13 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0
-
-package main
-
-import (
-	"github.com/hashicorp/vault/tools/gonilnilfunctions/pkg/analyzer"
-	"golang.org/x/tools/go/analysis/singlechecker"
-)
-
-func main() {
-	singlechecker.Main(analyzer.Analyzer)
-}