Switch linting to GitHub Actions (#1373)

* Switch linting to GitHub Actions

Also fix issues identified by linter

* Spelling

Author: sethvargo

.github/workflows/codereview.yaml

@@ -6,17 +6,35 @@ on:
     - 'main'
 
 jobs:
-  only-user-errors:
-    name: errorw-bot
+  golangci-lint:
+    name: go-lint
     runs-on: ubuntu-latest
 
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v2
+
+      - name: lint
+        uses: golangci/golangci-lint-action@v2
+        with:
+          version: v1.36.0
+          only-new-issues: true
+
+  reviewdog:
+    name: reviewdog
+    runs-on: ubuntu-latest
+    continue-on-error: true
+
     steps:
       - name: Check out code
         uses: actions/checkout@v2
 
       - uses: reviewdog/action-setup@v1
 
-      - name: Check
+      - name: Install pcregrep
+        run: sudo apt-get -yqq install pcregrep
+
+      - name: errorw
         shell: bash
         env:
           REVIEWDOG_GITHUB_API_TOKEN: ${{ github.token }}
@@ -32,17 +50,7 @@ jobs:
             -fail-on-error="false" \
             -level="warning"
 
-  copyright-check:
-    name: copyright-check
-    runs-on: ubuntu-latest
-
-    steps:
-      - name: Check out code
-        uses: actions/checkout@v2
-
-      - uses: reviewdog/action-setup@v1
-
-      - name: Check
+      - name: copyright-check
         shell: bash
         env:
           REVIEWDOG_GITHUB_API_TOKEN: ${{ github.token }}
@@ -59,6 +67,23 @@ jobs:
             -fail-on-error="true" \
             -level="error"
 
+      - name: tab-check
+        shell: bash
+        env:
+          REVIEWDOG_GITHUB_API_TOKEN: ${{ github.token }}
+        run: |-
+          set -eEu
+          set +o pipefail
+
+          YEAR=$(date +"%Y")
+          make tabcheck | \
+          reviewdog -efm="%f:%l:%m" \
+            -name="tab-check" \
+            -reporter="github-pr-review" \
+            -filter-mode="diff_context" \
+            -fail-on-error="true" \
+            -level="error"
+
   zap-logger:
     name: zap-logger
     runs-on: ubuntu-latest

.golangci.yaml

@@ -0,0 +1,94 @@
+# Copyright 2021 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+run:
+  # default: '1m'
+  timeout: '5m'
+
+  # default: []
+  build-tags:
+  - 'all'
+
+  # default: []
+  skip-dirs:
+  - 'internal/pb'
+
+  # default: true
+  skip-dirs-use-default: false
+
+  # default: ''
+  modules-download-mode: 'readonly'
+
+  # default: false
+  allow-parallel-runners: true
+
+linters:
+  enable:
+  - 'asciicheck'
+  - 'bodyclose'
+  - 'deadcode'
+  - 'depguard'
+  - 'dogsled'
+  - 'errcheck'
+  - 'errorlint'
+  - 'exportloopref'
+  - 'gofmt'
+  - 'gofumpt'
+  - 'goheader'
+  - 'goimports'
+  - 'golint'
+  - 'gomodguard'
+  - 'goprintffuncname'
+  - 'gosec'
+  - 'gosimple'
+  - 'govet'
+  - 'ineffassign'
+  - 'interfacer'
+  - 'makezero'
+  - 'misspell'
+  - 'noctx'
+  - 'nolintlint'
+  - 'paralleltest'
+  - 'prealloc'
+  - 'predeclared'
+  - 'scopelint'
+  - 'sqlclosecheck'
+  - 'staticcheck'
+  - 'structcheck'
+  - 'stylecheck'
+  - 'typecheck'
+  - 'unconvert'
+  - 'unused'
+  - 'varcheck'
+  - 'whitespace'
+
+issues:
+  # default: []
+  exclude:
+  - '^S1023:' # staticcheck: redundant returns help with http handlers
+  - '^SA3000:' # staticcheck: not required in Go 11.4+
+  - '^G102:' # gosec: we have to bind to all ifaces
+  - '^G402:' # gosec: some services terminate at the load balancer
+  - '^G505:' # gosec: we use crypto/sha1 for some HMACs
+  - '^Range statement' # paralleltest: false positives
+
+  # default: 50
+  max-issues-per-linter: 0
+
+  # default: 3
+  max-same-issues: 0
+
+severity:
+  # default: ''
+  default-severity: error

Makefile

@@ -12,51 +12,32 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-VETTERS = "asmdecl,assign,atomic,bools,buildtag,cgocall,composites,copylocks,errorsas,httpresponse,loopclosure,lostcancel,nilfunc,printf,shift,stdmethods,structtag,tests,unmarshal,unreachable,unsafeptr,unusedresult"
 GOFMT_FILES = $(shell go list -f '{{.Dir}}' ./... | grep -v '/pb')
+HTML_FILES = $(shell find . -name \*.html)
 GO_FILES = $(shell find . -name \*.go)
-
-bodyclose:
-	@command -v bodyclose > /dev/null 2>&1 || go get github.com/timakin/bodyclose
-	@go vet -tags=all -vettool=$$(which bodyclose) ./...
-.PHONY: bodyclose
-
-fmtcheck:
-	@command -v goimports > /dev/null 2>&1 || go get golang.org/x/tools/cmd/goimports
-	@CHANGES="$$(goimports -d $(GOFMT_FILES))"; \
-		if [ -n "$${CHANGES}" ]; then \
-			echo "Unformatted (run goimports -w .):\n\n$${CHANGES}\n\n"; \
-			exit 1; \
-		fi
-	@# Annoyingly, goimports does not support the simplify flag.
-	@CHANGES="$$(gofmt -s -d $(GOFMT_FILES))"; \
-		if [ -n "$${CHANGES}" ]; then \
-			echo "Unformatted (run gofmt -s -w .):\n\n$${CHANGES}\n\n"; \
+MD_FILES = $(shell find . -name \*.md)
+
+# lint uses the same linter as CI and tries to report the same results running
+# locally. There is a chance that CI detects linter errors that are not found
+# locally, but it should be rare.
+lint:
+	@command -v golangci-lint > /dev/null 2>&1 || GO111MODULE=off go get github.com/golangci/golangci-lint/cmd/[email protected]
+	golangci-lint run --config .golangci.yaml
+.PHONY: lint
+
+tabcheck:
+	@FINDINGS="$$(awk '/\t/ {printf "%s:%s:found tab character\n",FILENAME,FNR}' $(HTML_FILES))"; \
+		if [ -n "$${FINDINGS}" ]; then \
+			echo "$${FINDINGS}\n\n"; \
 			exit 1; \
 		fi
-.PHONY: fmtcheck
-
-spellcheck:
-	@command -v misspell > /dev/null 2>&1 || go get github.com/client9/misspell/cmd/misspell
-	@misspell -locale="US" -error -source="text" **/*
-.PHONY: spellcheck
-
-# SA3000 is not required in Go 1.15+: https://github.com/dominikh/go-tools/issues/708
-staticcheck:
-	@command -v staticcheck > /dev/null 2>&1 || go get honnef.co/go/tools/cmd/staticcheck
-	@staticcheck -tags=all -checks="all,-SA3000" -tests $(GOFMT_FILES)
-.PHONY: staticcheck
-
-zapcheck:
-	@command -v zapw > /dev/null 2>&1 || GO111MODULE=off go get github.com/sethvargo/zapw/cmd/zapw
-	@zapw ./...
+.PHONY: tabcheck
 
 test:
 	@go test \
 		-count=1 \
 		-short \
 		-timeout=5m \
-		-vet="${VETTERS}" \
 		./...
 .PHONY: test
 
@@ -65,11 +46,15 @@ test-acc:
 		-count=1 \
 		-race \
 		-timeout=10m \
-		-vet="${VETTERS}" \
 		./... \
 		-coverprofile=coverage.out
 .PHONY: test-acc
 
 test-coverage:
 	@go tool cover -func ./coverage.out | grep total
 .PHONY: test-coverage
+
+zapcheck:
+	@command -v zapw > /dev/null 2>&1 || GO111MODULE=off go get github.com/sethvargo/zapw/cmd/zapw
+	@zapw ./...
+.PHONY: zapcheck

cmd/admin-console/templates/export.html

@@ -51,7 +51,7 @@
           <option value="false" {{if not .export.IncludeTravelers}}selected{{end}}>No</option>
         </select>
         <small class="form-text text-muted">
-          Should federeated-in traveler keys be included in this export. If this
+          Should federated-in traveler keys be included in this export. If this
           server is sharing keys with anouther server, the recommended setting
           is 'yes'. Even if you are not yet federating, but may in the future,
           'yes' is still the recommended setting.
@@ -65,9 +65,8 @@
           <option value="false" {{if not .export.OnlyNonTravelers}}selected{{end}}>No</option>
         </select>
         <small class="form-text text-muted">
-		  Should only federeated-in non-traveler keys be included in this
-		  export. Note, setting this with 'Include Travelers' will result in an
-		  error.
+          Should only federated-in non-traveler keys be included in this export.
+          Note, setting this with 'Include Travelers' will result in an error.
         </small>
       </div>
 
@@ -77,7 +76,7 @@
         <label for="exclude-regions">Exclude regions</label>
         <small class="form-text text-muted">
           One per line, leave blank for only using the output region (above).
-		  Allows you to exclude travelers from certain regions.
+          Allows you to exclude travelers from certain regions.
         </small>
       </div>
 

docs/playbooks/playbook_test.go

@@ -53,7 +53,7 @@ func allPlaybookNames(t *testing.T) []string {
 	if err != nil {
 		t.Fatalf("failed to find all playbooks: %s", err)
 	}
-	var ret []string
+	ret := make([]string, 0, len(files))
 	for _, x := range files {
 		basename := filepath.Base(x)
 		ret = append(ret, basename[:len(basename)-len(".md")])
@@ -71,7 +71,7 @@ func allAlertNames(t *testing.T) []string {
 	if diag.HasErrors() {
 		t.Fatalf("failed to decode body: %v", diag)
 	}
-	var ret []string
+	ret := make([]string, 0, len(c.Resources))
 	for _, res := range c.Resources {
 		if res.Type != "google_monitoring_alert_policy" {
 			continue
@@ -103,7 +103,7 @@ func setDiff(xs, ys []string) []string {
 			delete(s1, k)
 		}
 	}
-	var ret []string
+	ret := make([]string, 0, len(s1))
 	for k := range s1 {
 		ret = append(ret, k)
 	}

internal/admin/config.go

@@ -29,10 +29,12 @@ import (
 	"github.com/google/exposure-notifications-server/pkg/secrets"
 )
 
-var _ setup.BlobstoreConfigProvider = (*Config)(nil)
-var _ setup.DatabaseConfigProvider = (*Config)(nil)
-var _ setup.KeyManagerConfigProvider = (*Config)(nil)
-var _ setup.SecretManagerConfigProvider = (*Config)(nil)
+var (
+	_ setup.BlobstoreConfigProvider     = (*Config)(nil)
+	_ setup.DatabaseConfigProvider      = (*Config)(nil)
+	_ setup.KeyManagerConfigProvider    = (*Config)(nil)
+	_ setup.SecretManagerConfigProvider = (*Config)(nil)
+)
 
 type Config struct {
 	Database      database.Config

internal/admin/handle_health_authority.go

@@ -141,7 +141,6 @@ func (s *Server) HandleHealthAuthorityKeys() func(c *gin.Context) {
 				ErrorPage(c, fmt.Sprintf("Error saving health authority key: %v", err))
 				return
 			}
-
 		} else if action == "revoke" || action == "reinstate" || action == "activate" {
 			version := c.Param("version")
 
@@ -173,7 +172,6 @@ func (s *Server) HandleHealthAuthorityKeys() func(c *gin.Context) {
 				ErrorPage(c, fmt.Sprintf("Error saving health authority key: %v", err))
 				return
 			}
-
 		} else {
 			ErrorPage(c, "invalid action")
 			return

internal/authorizedapp/database/authorized_app.go

@@ -57,7 +57,6 @@ func (aa *AuthorizedAppDB) InsertAuthorizedApp(ctx context.Context, m *model.Aut
 		`, m.AppPackageName, m.AllAllowedRegions(),
 			m.AllAllowedHealthAuthorityIDs(), m.BypassHealthAuthorityVerification,
 			m.BypassRevisionToken)
-
 		if err != nil {
 			return fmt.Errorf("inserting authorizedapp: %w", err)
 		}

internal/authorizedapp/database/authorized_app_test.go

@@ -296,5 +296,4 @@ func TestListAuthorizedApps(t *testing.T) {
 	if diff := cmp.Diff(apps, got); diff != "" {
 		t.Errorf("mismatch (-want, +got):\n%s", diff)
 	}
-
 }

internal/authorizedapp/database_provider.go

@@ -80,7 +80,6 @@ func (p *DatabaseProvider) AppConfig(ctx context.Context, name string) (*model.A
 		return config, nil
 	}
 	cached, err := p.cache.WriteThruLookup(name, lookup)
-
 	// Indicates an error on the write thru lookup.
 	if err != nil {
 		return nil, err

internal/cleanup/config.go

@@ -25,10 +25,12 @@ import (
 )
 
 // Compile-time check to assert this config matches requirements.
-var _ setup.BlobstoreConfigProvider = (*Config)(nil)
-var _ setup.DatabaseConfigProvider = (*Config)(nil)
-var _ setup.SecretManagerConfigProvider = (*Config)(nil)
-var _ setup.ObservabilityExporterConfigProvider = (*Config)(nil)
+var (
+	_ setup.BlobstoreConfigProvider             = (*Config)(nil)
+	_ setup.DatabaseConfigProvider              = (*Config)(nil)
+	_ setup.SecretManagerConfigProvider         = (*Config)(nil)
+	_ setup.ObservabilityExporterConfigProvider = (*Config)(nil)
+)
 
 // Config represents the configuration and associated environment variables for
 // the cleanup components.

internal/database/connection.go

@@ -52,7 +52,7 @@ func NewFromEnv(ctx context.Context, cfg *Config) (*DB, error) {
 
 	pool, err := pgxpool.ConnectConfig(ctx, pgxConfig)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create connection pool: %v", err)
+		return nil, fmt.Errorf("failed to create connection pool: %w", err)
 	}
 
 	return &DB{Pool: pool}, nil