add linting & fix lint

Also add GitHub Action for lint

.github/workflows/code-quality.yml

@@ -0,0 +1,31 @@
+name: code quality
+
+on:
+  push:
+    branches: [ main ]
+  pull_request:
+    branches: [ main ]
+
+jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+
+    - name: set up Go
+      uses: actions/setup-go@v4
+      with:
+        go-version-file: 'go.mod'
+
+    - name: ensure go mod tidy
+      run: go mod tidy && git diff --exit-code go.mod go.sum
+
+    - name: get linter version
+      id: linter-version
+      run: echo "version=$(grep 'LINTER_VERSION :=' Makefile | awk '{print $3}')" > $GITHUB_OUTPUT
+
+    - name: lint
+      uses: golangci/golangci-lint-action@v3
+      with:
+        version: v${{ steps.linter-version.outputs.version }}
+        args: --timeout=15m

.golangci.yml

@@ -0,0 +1,157 @@
+linters-settings:
+  dupl:
+    threshold: 100
+  exhaustive:
+    default-signifies-exhaustive: true
+  funlen:
+    lines: 100
+    statements: 50
+  goconst:
+    min-len: 2
+    min-occurrences: 2
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - opinionated
+      - performance
+      - style
+  gocyclo:
+    min-complexity: 15
+  goimports:
+    local-prefixes: github.com/kylrth/api
+  gomnd:
+    settings:
+      mnd:
+        # don't include the "operation" and "assign"
+        checks: argument,case,condition,return
+  govet:
+    check-shadowing: true
+    enable-all: true
+    disable:
+      - fieldalignment
+  ireturn:
+    allow:
+      - error
+      - generic
+  lll:
+    line-length: 100
+  misspell:
+    locale: US
+  nolintlint:
+    allow-unused: false
+    allow-leading-space: false
+    require-explanation: true
+    require-specific: true
+
+linters:
+  disable-all: true
+  enable:
+    - asasalint
+    - asciicheck
+    - bidichk
+    - bodyclose
+    - containedctx
+    - contextcheck
+    - cyclop
+    - decorder
+    - dogsled
+    - dupl
+    - dupword
+    - durationcheck
+    - errcheck
+    - errchkjson
+    - errname
+    - errorlint
+    - execinquery
+    - exhaustive
+    - exportloopref
+    - forcetypeassert
+    - funlen
+    - ginkgolinter
+    - gocheckcompilerdirectives
+    - gocognit
+    - goconst
+    - gocritic
+    - gocyclo
+    - godot
+    - godox
+    - gofmt
+    - gofumpt
+    - goimports
+    - gomoddirectives
+    - gomodguard
+    - goprintffuncname
+    - gosec
+    - gosmopolitan
+    - gosimple
+    - grouper
+    - govet
+    - importas
+    - interfacebloat
+    - ineffassign
+    - ireturn
+    - lll
+    - loggercheck
+    - maintidx
+    - makezero
+    - mirror
+    - misspell
+    - musttag
+    - nakedret
+    - nestif
+    - nilerr
+    - nilnil
+    - nlreturn
+    - noctx
+    - nolintlint
+    - nosprintfhostport
+    - paralleltest
+    - prealloc
+    - predeclared
+    - promlinter
+    - reassign
+    - revive
+    - rowserrcheck
+    - sqlclosecheck
+    - staticcheck
+    - stylecheck
+    - tagalign
+    - tenv
+    - testableexamples
+    - testpackage
+    - thelper
+    - tparallel
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - usestdlibvars
+    - wastedassign
+    - whitespace
+    - zerologlint
+  # not enabled:
+  # - deadcode  # deprecated
+  # - depguard  # We have no restrictions on dependencies.
+  # - exhaustivestruct  # deprecated
+  # - exhaustruct  # This doesn't make sense to check for in most cases.
+  # - forbidigo  # nothing to forbid
+  # - gci  # prefer goimports
+  # - gochecknoglobals  # I don't think globals should be universally disallowed.
+  # - gochecknoinits  # I don't think init should be universally disallowed.
+  # - goerr113  # I think it's ok to include info in errors without defining a new type.
+  # - goheader  # no need for a header
+  # - golint  # deprecated
+  # - gomnd  # prefer goconst
+  # - ifshort  # deprecated
+  # - interfacer  # deprecated
+  # - maligned  # deprecated
+  # - nonamedreturns  # disagree
+  # - nosnakecase  # deprecated
+  # - scopelint  # deprecated
+  # - structcheck  # deprecated
+  # - tagliatelle  # We have to use the JSON fields from the API.
+  # - varcheck  # deprecated
+  # - varnamelen  # I don't think this matters.
+  # - wrapcheck  # I think this should only happen for public functions.
+  # - wsl  # not a fan

Makefile

@@ -0,0 +1,19 @@
+all: lint build
+
+BINDIR := bin
+
+## LINT
+
+LINTER_VERSION := 1.53.3
+LINTER := $(BINDIR)/golangci-lint_$(LINTER_VERSION)
+DEV_OS := $(shell uname -s | tr A-Z a-z)
+
+$(LINTER):
+	mkdir -p $(BINDIR)
+	wget "https://github.com/golangci/golangci-lint/releases/download/v$(LINTER_VERSION)/golangci-lint-$(LINTER_VERSION)-$(DEV_OS)-amd64.tar.gz" -O - \
+		| tar -xz -C $(BINDIR) --strip-components=1 --exclude=README.md --exclude=LICENSE
+	mv $(BINDIR)/golangci-lint $(LINTER)
+
+.PHONY: lint
+lint: $(LINTER)
+	$(LINTER) run --deadline=2m

cmd/admin.go

@@ -13,7 +13,6 @@ import (
 var adminCmd = &cobra.Command{
 	Use:   "admin",
 	Short: "Manage admins",
-	Run:   noRun,
 }
 
 func init() {

cmd/root.go

@@ -18,12 +18,6 @@ func main() {
 
 var rootCmd = &cobra.Command{
 	Short: "Avoid Panic! at the Disco(rd)",
-	Run:   noRun,
-}
-
-func noRun(cmd *cobra.Command, args []string) {
-	fmt.Fprintf(os.Stderr, "Run the `help` subcommand for usage.")
-	os.Exit(2)
 }
 
 var verbosity int

cmd/server.go

@@ -2,11 +2,13 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"os"
 
 	"github.com/cobaltspeech/log"
 	"github.com/gofiber/fiber/v2"
 	"github.com/gofiber/fiber/v2/middleware/logger"
+	_ "github.com/golang-migrate/migrate/v4/database/postgres"
 	"github.com/jackc/pgx/v5/pgxpool"
 	"github.com/spf13/cobra"
 
@@ -17,11 +19,14 @@ import (
 func withLAndDB(f func(log.Logger, *pgxpool.Pool, []string) error) func(*cobra.Command, []string) {
 	return withLogger(func(l log.Logger, args []string) error {
 		pgURI := os.Getenv("DATABASE_URL")
-		db.ApplyMigrations(pgURI)
+		err := db.ApplyMigrations(pgURI)
+		if err != nil {
+			return fmt.Errorf("apply database migrations: %w", err)
+		}
 
 		pool, err := pgxpool.New(context.Background(), os.Getenv("DATABASE_URL"))
 		if err != nil {
-			return err
+			return fmt.Errorf("connect to database: %w", err)
 		}
 		defer pool.Close()
 

internal/db/auth.go

@@ -7,19 +7,18 @@ import (
 
 	"github.com/cobaltspeech/log"
 	"github.com/jackc/pgx/v5"
-	"github.com/jackc/pgx/v5/pgxpool"
 	"golang.org/x/crypto/bcrypt"
 )
 
 // AdminTable represents the table of admins permitted to use the service. It handles password
 // hashing, so all `pass` method arguments are expected to be plaintext.
 type AdminTable struct {
 	logger log.Logger
-	pool   pgxIface
+	pool   PgxIface
 }
 
 // NewAdminTable creates a new AdminTable backed by a Postgres connection pool.
-func NewAdminTable(l log.Logger, pool *pgxpool.Pool) *AdminTable {
+func NewAdminTable(l log.Logger, pool PgxIface) *AdminTable {
 	out := AdminTable{
 		logger: l,
 		pool:   pool,

internal/db/auth_test.go

@@ -1,32 +1,31 @@
-package db
+package db_test
 
 import (
 	"context"
 	"errors"
-	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/cobaltspeech/log/pkg/testinglog"
 	"github.com/jackc/pgx/v5"
+	"github.com/kylrth/disco-bouncer/internal/db"
 	"github.com/pashagolub/pgxmock/v2"
 )
 
-// AnyBcrypt satisfies pgxmock.Argument
+// AnyBcrypt satisfies pgxmock.Argument.
 type AnyBcrypt struct{}
 
-// Match matches any string that starts with "$2a$10$"
+// Match matches any string that starts with "$2a$10$".
 func (a AnyBcrypt) Match(v interface{}) bool {
 	switch val := v.(type) {
 	case string:
-		fmt.Println(val)
 		return strings.HasPrefix(val, "$2a$10$")
 	default:
 		return false
 	}
 }
 
-func TestAdminTable(t *testing.T) {
+func TestAdminTable(t *testing.T) { //nolint:cyclop,funlen,gocyclo // testing sequential calls
 	t.Parallel()
 
 	mockDB, err := pgxmock.NewPool()
@@ -36,7 +35,7 @@ func TestAdminTable(t *testing.T) {
 	defer mockDB.Close()
 
 	logger := testinglog.NewConvenientLogger(t)
-	table := &AdminTable{logger: logger, pool: mockDB}
+	table := db.NewAdminTable(logger, mockDB)
 	ctx := context.Background()
 
 	// create admin John and check password
@@ -113,7 +112,7 @@ func TestAdminTable(t *testing.T) {
 		WithArgs("stephen", AnyBcrypt{}).
 		WillReturnResult(pgxmock.NewResult("UPDATE", 0))
 	err = table.ChangePassword(ctx, "stephen", "password")
-	if !errors.Is(err, ErrNoUser) {
+	if !errors.Is(err, db.ErrNoUser) {
 		t.Errorf("unexpected error from ChangePassword: %v", err)
 	}
 
@@ -122,7 +121,7 @@ func TestAdminTable(t *testing.T) {
 		WithArgs("stephen").
 		WillReturnResult(pgxmock.NewResult("DELETE", 0))
 	err = table.DeleteAdmin(ctx, "stephen")
-	if !errors.Is(err, ErrNoUser) {
+	if !errors.Is(err, db.ErrNoUser) {
 		t.Errorf("unexpected error from DeleteAdmin: %v", err)
 	}
 

internal/db/migrate.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 
 	"github.com/golang-migrate/migrate/v4"
-	_ "github.com/golang-migrate/migrate/v4/database/postgres"
 	"github.com/golang-migrate/migrate/v4/source/iofs"
 )
 

internal/db/mock.go

@@ -7,8 +7,8 @@ import (
 	"github.com/jackc/pgx/v5/pgconn"
 )
 
-// pgxIface allows for mocking a *pgxpool.Pool.
-type pgxIface interface {
+// PgxIface allows the Table objects to use anything that looks like a *pgxpool.Pool.
+type PgxIface interface {
 	Begin(context.Context) (pgx.Tx, error)
 	Exec(context.Context, string, ...interface{}) (pgconn.CommandTag, error)
 	QueryRow(context.Context, string, ...interface{}) pgx.Row

internal/db/users.go

@@ -6,17 +6,16 @@ import (
 
 	"github.com/cobaltspeech/log"
 	"github.com/jackc/pgx/v5"
-	"github.com/jackc/pgx/v5/pgxpool"
 )
 
 // UserTable represents the table of users that the bouncer will accept into the Discord server.
 type UserTable struct {
 	logger log.Logger
-	pool   pgxIface
+	pool   PgxIface
 }
 
 // NewUserTable creates a new UserTable backed by a Postgres connection pool.
-func NewUserTable(l log.Logger, pool *pgxpool.Pool) *UserTable {
+func NewUserTable(l log.Logger, pool PgxIface) *UserTable {
 	out := UserTable{
 		logger: l,
 		pool:   pool,