Upgrade golangci-lint to v2

For better or rwose, golangci-lint has released a v2 [1] that's
incompatible with v1. It'll only be a matter of time until we're forced
to upgrade to it, so I figure we may as well do it early.

There's a bunch of new obnoxious linters that I've disabled. Most of
these complain about "cyclomatic complexity" or "cognitive complexity"
or functions being too long (they all seem to want everything broken
down into functions of ten lines).

Aside from that, `testifylint` seems to have gotten even more effective
at finding misuses of testify, so it fixed a bunch more arguable misuses
of various assertions, which seems good for normalizing code.

The structure of the YAML file changes fairly significantly, but it's
all basically the same thing with various aspects reorganized. I made
the new one using the built-in `golangci-lint migrate` and copied
comments over manually.

If we can get this in, the next step will be to distribute it out to
other River projects. Most of them are using copies of this repo's
configuration file, so it should be pretty easy to do.

[1] https://ldez.github.io/blog/2025/03/23/golangci-lint-v2/
[2] golangci/golangci-lint#5606

Author: brandur

.github/workflows/ci.yaml

@@ -218,7 +218,7 @@ jobs:
     name: lint
     runs-on: ubuntu-latest
     env:
-      GOLANGCI_LINT_VERSION: v1.64.4
+      GOLANGCI_LINT_VERSION: v2.0.0
     permissions:
       contents: read
       # allow read access to pull request. Use with `only-new-issues` option.
@@ -234,7 +234,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Lint
-        uses: golangci/golangci-lint-action@v4
+        uses: golangci/golangci-lint-action@v7
         with:
           # golangci-lint needs to be run separately for every Go module, and
           # its GitHub Action doesn't provide any way to do that. Have it fetch

.golangci.yaml

@@ -1,16 +1,7 @@
-issues:
-  exclude:
-    - 'Error return value of .(\w+\.Rollback(.*)). is not checked'
+version: "2"
 
 linters:
-  presets:
-    - bugs
-    - comment
-    - format
-    - performance
-    - style
-    - test
-    - unused
+  default: all
 
   disable:
     # disabled, but which we should enable with discussion
@@ -21,82 +12,108 @@ linters:
     - testpackage # requires tests in test packages like `river_test`
 
     # disabled because they're annoying/bad
+    - cyclop # screams into the void at "cyclomatic complexity"
+    - funlen # screams when functions are more than 60 lines long; what are we even doing here guys
     - interfacebloat # we do in fact want >10 methods on the Adapter interface or wherever we see fit.
+    - gocognit # yells that "cognitive complexity" is too high; why
+    - gocyclo # ANOTHER "cyclomatic complexity" checker (see also "cyclop" and "gocyclo")
     - godox # bans TODO statements; total non-starter at the moment
     - err113 # wants all errors to be defined as variables at the package level; quite obnoxious
+    - maintidx # ANOTHER ANOTHER "cyclomatic complexity" lint (see also "cyclop" and "gocyclo")
     - mnd # detects "magic numbers", which it defines as any number; annoying
+    - nestif # yells when if blocks are nested; what planet do these people come from?
     - ireturn # bans returning interfaces; questionable as is, but also buggy as hell; very, very annoying
     - lll # restricts maximum line length; annoying
     - nlreturn # requires a blank line before returns; annoying
     - wsl # a bunch of style/whitespace stuff; annoying
 
-linters-settings:
-  depguard:
-    rules:
-      all:
-        files: ["$all"]
-        allow:
-        deny:
-          - desc: "Use `github.com/google/uuid` package for UUIDs instead."
-            pkg: "github.com/xtgo/uuid"
-      not-test:
-        files: ["!$test"]
-        deny:
-          - desc: "Don't use `dbadaptertest` package outside of test environments."
-            pkg: "github.com/riverqueue/river/internal/dbadaptertest"
-          - desc: "Don't use `riverinternaltest` package outside of test environments."
-            pkg: "github.com/riverqueue/river/internal/riverinternaltest"
+  settings:
+    depguard:
+      rules:
+        all:
+          files:
+            - $all
+          deny:
+            - pkg: github.com/xtgo/uuid
+              desc: Use `github.com/google/uuid` package for UUIDs instead.
+        not-test:
+          files:
+            - "!$test"
+          deny:
+            - pkg: github.com/riverqueue/river/internal/dbadaptertest
+              desc: Don't use `dbadaptertest` package outside of test environments.
+            - pkg: github.com/riverqueue/river/internal/riverinternaltest
+              desc: Don't use `riverinternaltest` package outside of test environments.
 
-  forbidigo:
-    forbid:
-      - msg: "Use `require` variants instead."
-        p: '^assert\.'
-      - msg: "Use `Func` suffix for function variables instead."
-        p: 'Fn\b'
-      - msg: "Use built-in `max` function instead."
-        p: '\bmath\.Max\b'
-      - msg: "Use built-in `min` function instead."
-        p: '\bmath\.Min\b'
+    forbidigo:
+      forbid:
+        - pattern: ^assert\.
+          msg: Use `require` variants instead.
+        - pattern: Fn\b
+          msg: Use `Func` suffix for function variables instead.
+        - pattern: \bmath\.Max\b
+          msg: Use built-in `max` function instead.
+        - pattern: \bmath\.Min\b
+          msg: Use built-in `min` function instead.
 
-  gci:
-    sections:
-      - Standard
-      - Default
-      - Prefix(github.com/riverqueue)
+    gomoddirectives:
+      replace-local: true
 
-  gomoddirectives:
-    replace-local: true
+    gosec:
+      excludes:
+        - G404 # use of non-crypto random; overly broad for our use case
 
-  gosec:
-    excludes:
-      - G404 # use of non-crypto random; overly broad for our use case
+    revive:
+      rules:
+        - name: unused-parameter
+          disabled: true
 
-  revive:
-    rules:
-      - name: unused-parameter
-        disabled: true
+    tagliatelle:
+      case:
+        rules:
+          json: snake
 
-  tagliatelle:
-    case:
-      rules:
-        json: snake
+    testifylint:
+      enable-all: true
+      disable:
+        - go-require
+
+    varnamelen:
+      ignore-names:
+        - db
+        - eg
+        - f
+        - i
+        - id
+        - j
+        - mu
+        - sb # common convention for string builder
+        - t
+        - tt # common convention for table tests
+        - tx
+        - wg
+
+  exclusions:
+    generated: lax
+    presets:
+      - comments
+      - common-false-positives
+      - legacy
+      - std-error-handling
+    rules:
+      - path: (.+)\.go$
+        text: Error return value of .(\w+\.Rollback(.*)). is not checked
 
-  testifylint:
-    enable-all: true
-    disable:
-      - go-require
+formatters:
+  enable:
+    - gci
+    - gofmt
+    - gofumpt
+    - goimports
 
-  varnamelen:
-    ignore-names:
-      - db
-      - eg
-      - f
-      - i
-      - id
-      - j
-      - mu
-      - sb # common convention for string builder
-      - t
-      - tt # common convention for table tests
-      - tx
-      - wg
+  settings:
+    gci:
+      sections:
+        - Standard
+        - Default
+        - Prefix(github.com/riverqueue)

error_test.go

@@ -31,7 +31,7 @@ func TestUnknownJobKindError_As(t *testing.T) {
 		t.Parallel()
 
 		var err *river.UnknownJobKindError
-		require.False(t, errors.As(errors.New("some other error"), &err))
+		require.NotErrorAs(t, errors.New("some other error"), &err)
 	})
 }
 

internal/dblist/db_list.go

@@ -99,10 +99,12 @@ func JobList(ctx context.Context, exec riverdriver.Executor, params *JobListPara
 
 	for i, orderBy := range params.OrderBy {
 		orderByBuilder.WriteString(orderBy.Expr)
-		if orderBy.Order == SortOrderAsc {
+		switch orderBy.Order {
+		case SortOrderAsc:
 			orderByBuilder.WriteString(" ASC")
-		} else if orderBy.Order == SortOrderDesc {
+		case SortOrderDesc:
 			orderByBuilder.WriteString(" DESC")
+		case SortOrderUnspecified:
 		}
 		if i < len(params.OrderBy)-1 {
 			orderByBuilder.WriteString(", ")

internal/jobexecutor/job_executor_test.go

@@ -233,7 +233,7 @@ func TestJobExecutor_Execute(t *testing.T) {
 
 		executor, bundle := setup(t)
 
-		now := executor.Archetype.Time.StubNowUTC(time.Now().UTC())
+		now := executor.Time.StubNowUTC(time.Now().UTC())
 
 		workerErr := errors.New("job error")
 		executor.WorkUnit = newWorkUnitFactoryWithCustomRetry(func() error { return workerErr }, nil).MakeUnit(bundle.jobRow)
@@ -249,7 +249,7 @@ func TestJobExecutor_Execute(t *testing.T) {
 		require.Equal(t, now.Truncate(1*time.Microsecond), job.Errors[0].At.Truncate(1*time.Microsecond))
 		require.Equal(t, 1, job.Errors[0].Attempt)
 		require.Equal(t, "job error", job.Errors[0].Error)
-		require.Equal(t, "", job.Errors[0].Trace)
+		require.Empty(t, job.Errors[0].Trace)
 	})
 
 	t.Run("ErrorAgainAfterRetry", func(t *testing.T) {
@@ -361,7 +361,7 @@ func TestJobExecutor_Execute(t *testing.T) {
 		require.WithinDuration(t, time.Now(), job.Errors[0].At, 2*time.Second)
 		require.Equal(t, 1, job.Errors[0].Attempt)
 		require.Equal(t, "JobCancelError: throw away this job", job.Errors[0].Error)
-		require.Equal(t, "", job.Errors[0].Trace)
+		require.Empty(t, job.Errors[0].Trace)
 	})
 
 	t.Run("JobSnoozeErrorReschedulesJobAndDecrementsAttempt", func(t *testing.T) {
@@ -721,7 +721,7 @@ func TestJobExecutor_Execute(t *testing.T) {
 		require.Equal(t, 1, job.Errors[0].Attempt)
 		require.Equal(t, "JobCancelError: job cancelled remotely", job.Errors[0].Error)
 		require.Equal(t, rivertype.ErrJobCancelledRemotely.Error(), job.Errors[0].Error)
-		require.Equal(t, "", job.Errors[0].Trace)
+		require.Empty(t, job.Errors[0].Trace)
 	})
 
 	t.Run("RemoteCancellationJobNotCancelledIfNoErrorReturned", func(t *testing.T) {

job_list_params_test.go

@@ -25,8 +25,8 @@ func Test_JobListCursor_JobListCursorFromJob(t *testing.T) {
 	cursor := JobListCursorFromJob(jobRow)
 	require.Zero(t, cursor.id)
 	require.Equal(t, jobRow, cursor.job)
-	require.Zero(t, cursor.kind)
-	require.Zero(t, cursor.queue)
+	require.Empty(t, cursor.kind)
+	require.Empty(t, cursor.queue)
 	require.Zero(t, cursor.sortField)
 	require.Zero(t, cursor.time)
 }
@@ -163,7 +163,7 @@ func Test_JobListCursor_MarshalJSON(t *testing.T) {
 
 		text, err := json.Marshal(cursor)
 		require.NoError(t, err)
-		require.NotEqual(t, "", text)
+		require.NotEmpty(t, text)
 
 		unmarshaledParams := &JobListCursor{}
 		require.NoError(t, json.Unmarshal(text, unmarshaledParams))

producer.go

@@ -525,10 +525,7 @@ func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan pr
 func (p *producer) executorShutdownLoop() {
 	// No more jobs will be fetched or executed. However, we must wait for all
 	// in-progress jobs to complete.
-	for {
-		if len(p.activeJobs) == 0 {
-			break
-		}
+	for len(p.activeJobs) != 0 {
 		result := <-p.jobResultCh
 		p.removeActiveJob(result.ID)
 	}

rivershared/testfactory/test_factory.go

@@ -46,7 +46,7 @@ func Job(ctx context.Context, tb testing.TB, exec riverdriver.Executor, opts *Jo
 	return job
 }
 
-func Job_Build(tb testing.TB, opts *JobOpts) *riverdriver.JobInsertFullParams { //nolint:stylecheck
+func Job_Build(tb testing.TB, opts *JobOpts) *riverdriver.JobInsertFullParams {
 	tb.Helper()
 
 	encodedArgs := opts.EncodedArgs