From facf2bad513d1fc9269210b9ed801a8f4edbed00 Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 24 Mar 2025 21:59:50 -0700 Subject: [PATCH] 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/ --- .github/workflows/ci.yaml | 4 +- .golangci.yaml | 164 ++++++++++++---------- error_test.go | 2 +- internal/dblist/db_list.go | 7 +- internal/jobexecutor/job_executor_test.go | 8 +- job_list_params_test.go | 6 +- producer.go | 5 +- rivershared/testfactory/test_factory.go | 2 +- rivertest/worker_test.go | 84 +++++------ rivertype/river_type.go | 4 + 10 files changed, 154 insertions(+), 132 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d3e18626..78432bf7 100644 --- a/.github/workflows/ci.yaml +++ b/.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 diff --git a/.golangci.yaml b/.golangci.yaml index 1872697a..a5773f09 100644 --- a/.golangci.yaml +++ b/.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,109 @@ 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: + - 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 - 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: + - msg: Use `require` variants instead. + pattern: ^assert\. + - msg: Use `Func` suffix for function variables instead. + pattern: Fn\b + - msg: Use built-in `max` function instead. + pattern: \bmath\.Max\b + - msg: Use built-in `min` function instead. + pattern: \bmath\.Min\b - 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 + - r + - sb # common convention for string builder + - t + - tt # common convention for table tests + - tx + - w + - 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) + - Prefix(riverqueue.com/riverpro) diff --git a/error_test.go b/error_test.go index a1f3ea3e..8a1533c7 100644 --- a/error_test.go +++ b/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) }) } diff --git a/internal/dblist/db_list.go b/internal/dblist/db_list.go index e6bd6bab..9498a86d 100644 --- a/internal/dblist/db_list.go +++ b/internal/dblist/db_list.go @@ -99,10 +99,13 @@ 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: + return nil, errors.New("should not have gotten SortOrderUnspecified by this point before executing list (bug?)") } if i < len(params.OrderBy)-1 { orderByBuilder.WriteString(", ") diff --git a/internal/jobexecutor/job_executor_test.go b/internal/jobexecutor/job_executor_test.go index 694986d5..3c6e58f4 100644 --- a/internal/jobexecutor/job_executor_test.go +++ b/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) { diff --git a/job_list_params_test.go b/job_list_params_test.go index c5f7ce7f..acc5c758 100644 --- a/job_list_params_test.go +++ b/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)) diff --git a/producer.go b/producer.go index bec1657f..18665f97 100644 --- a/producer.go +++ b/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) } diff --git a/rivershared/testfactory/test_factory.go b/rivershared/testfactory/test_factory.go index 568e24b6..2d6b0b6f 100644 --- a/rivershared/testfactory/test_factory.go +++ b/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 diff --git a/rivertest/worker_test.go b/rivertest/worker_test.go index dded3f16..3a28b6a6 100644 --- a/rivertest/worker_test.go +++ b/rivertest/worker_test.go @@ -100,24 +100,24 @@ func TestWorker_Work(t *testing.T) { worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { require.Equal(t, testArgs{Value: "test"}, job.Args) - require.Equal(t, 1, job.JobRow.Attempt) - require.NotNil(t, job.JobRow.AttemptedAt) - require.WithinDuration(t, time.Now(), *job.JobRow.AttemptedAt, 5*time.Second) - require.Equal(t, []string{"rivertest-worker"}, job.JobRow.AttemptedBy) - require.WithinDuration(t, time.Now(), job.JobRow.CreatedAt, 5*time.Second) - require.JSONEq(t, `{"value": "test"}`, string(job.JobRow.EncodedArgs)) - require.Empty(t, job.JobRow.Errors) - require.Nil(t, job.JobRow.FinalizedAt) - require.Positive(t, job.JobRow.ID) - require.Equal(t, "rivertest_work_test", job.JobRow.Kind) - require.Equal(t, river.MaxAttemptsDefault, job.JobRow.MaxAttempts) - require.Equal(t, []byte(`{}`), job.JobRow.Metadata) - require.Equal(t, river.PriorityDefault, job.JobRow.Priority) - require.Equal(t, river.QueueDefault, job.JobRow.Queue) - require.WithinDuration(t, time.Now(), job.JobRow.ScheduledAt, 2*time.Second) - require.Equal(t, rivertype.JobStateRunning, job.JobRow.State) - require.Equal(t, []string{}, job.JobRow.Tags) - require.Nil(t, job.JobRow.UniqueKey) + require.Equal(t, 1, job.Attempt) + require.NotNil(t, job.AttemptedAt) + require.WithinDuration(t, time.Now(), *job.AttemptedAt, 5*time.Second) + require.Equal(t, []string{"rivertest-worker"}, job.AttemptedBy) + require.WithinDuration(t, time.Now(), job.CreatedAt, 5*time.Second) + require.JSONEq(t, `{"value": "test"}`, string(job.EncodedArgs)) + require.Empty(t, job.Errors) + require.Nil(t, job.FinalizedAt) + require.Positive(t, job.ID) + require.Equal(t, "rivertest_work_test", job.Kind) + require.Equal(t, river.MaxAttemptsDefault, job.MaxAttempts) + require.Equal(t, []byte(`{}`), job.Metadata) + require.Equal(t, river.PriorityDefault, job.Priority) + require.Equal(t, river.QueueDefault, job.Queue) + require.WithinDuration(t, time.Now(), job.ScheduledAt, 2*time.Second) + require.Equal(t, rivertype.JobStateRunning, job.State) + require.Equal(t, []string{}, job.Tags) + require.Nil(t, job.UniqueKey) _, hasContextKeyInsideTestWorker := ctx.Value(execution.ContextKeyInsideTestWorker{}).(bool) require.True(t, hasContextKeyInsideTestWorker) @@ -156,23 +156,23 @@ func TestWorker_Work(t *testing.T) { worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { require.Equal(t, testArgs{Value: "test3"}, job.Args) - require.Equal(t, 1, job.JobRow.Attempt) - require.NotNil(t, job.JobRow.AttemptedAt) - require.WithinDuration(t, time.Now().UTC(), *job.JobRow.AttemptedAt, 2*time.Second) - require.Equal(t, []string{"rivertest-worker"}, job.JobRow.AttemptedBy) - require.WithinDuration(t, time.Now().UTC(), job.JobRow.CreatedAt, 2*time.Second) - require.JSONEq(t, `{"value": "test3"}`, string(job.JobRow.EncodedArgs)) - require.Empty(t, job.JobRow.Errors) - require.Nil(t, job.JobRow.FinalizedAt) - require.Positive(t, job.JobRow.ID) - require.Equal(t, "rivertest_work_test", job.JobRow.Kind) - require.Equal(t, 420, job.JobRow.MaxAttempts) - require.JSONEq(t, `{"key": "value"}`, string(job.JobRow.Metadata)) - require.Equal(t, 3, job.JobRow.Priority) - require.Equal(t, "custom_queue", job.JobRow.Queue) - require.WithinDuration(t, hourFromNow, job.JobRow.ScheduledAt, 2*time.Second) - require.Equal(t, rivertype.JobStateRunning, job.JobRow.State) - require.Equal(t, []string{"tag1", "tag2"}, job.JobRow.Tags) + require.Equal(t, 1, job.Attempt) + require.NotNil(t, job.AttemptedAt) + require.WithinDuration(t, time.Now().UTC(), *job.AttemptedAt, 2*time.Second) + require.Equal(t, []string{"rivertest-worker"}, job.AttemptedBy) + require.WithinDuration(t, time.Now().UTC(), job.CreatedAt, 2*time.Second) + require.JSONEq(t, `{"value": "test3"}`, string(job.EncodedArgs)) + require.Empty(t, job.Errors) + require.Nil(t, job.FinalizedAt) + require.Positive(t, job.ID) + require.Equal(t, "rivertest_work_test", job.Kind) + require.Equal(t, 420, job.MaxAttempts) + require.JSONEq(t, `{"key": "value"}`, string(job.Metadata)) + require.Equal(t, 3, job.Priority) + require.Equal(t, "custom_queue", job.Queue) + require.WithinDuration(t, hourFromNow, job.ScheduledAt, 2*time.Second) + require.Equal(t, rivertype.JobStateRunning, job.State) + require.Equal(t, []string{"tag1", "tag2"}, job.Tags) return nil }) @@ -205,8 +205,8 @@ func TestWorker_Work(t *testing.T) { bundle.config.Test.Time = stubTime worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { - require.Empty(t, job.JobRow.UniqueKey) - require.Empty(t, job.JobRow.UniqueStates) + require.Empty(t, job.UniqueKey) + require.Empty(t, job.UniqueStates) return nil }) tw := NewWorker(t, bundle.driver, bundle.config, worker) @@ -258,9 +258,9 @@ func TestWorker_Work(t *testing.T) { bundle.config.Test.Time = timeStub worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error { - require.WithinDuration(t, hourFromNow, *job.JobRow.AttemptedAt, time.Millisecond) - require.WithinDuration(t, hourFromNow, job.JobRow.CreatedAt, time.Millisecond) - require.WithinDuration(t, hourFromNow, job.JobRow.ScheduledAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, *job.AttemptedAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, job.CreatedAt, time.Millisecond) + require.WithinDuration(t, hourFromNow, job.ScheduledAt, time.Millisecond) return nil }) tw := NewWorker(t, bundle.driver, bundle.config, worker) @@ -373,8 +373,8 @@ func TestWorker_WorkJob(t *testing.T) { testWorker, bundle := setup(t) bundle.workFunc = func(ctx context.Context, job *river.Job[testArgs]) error { - require.WithinDuration(t, time.Now(), *job.JobRow.AttemptedAt, 5*time.Second) - require.Equal(t, []string{"rivertest-workjob"}, job.JobRow.AttemptedBy) + require.WithinDuration(t, time.Now(), *job.AttemptedAt, 5*time.Second) + require.Equal(t, []string{"rivertest-workjob"}, job.AttemptedBy) require.Equal(t, rivertype.JobStateRunning, job.State) return nil } diff --git a/rivertype/river_type.go b/rivertype/river_type.go index ee416c04..d1bbda48 100644 --- a/rivertype/river_type.go +++ b/rivertype/river_type.go @@ -300,6 +300,8 @@ type JobInsertParams struct { // List of hook interfaces that may be implemented: // - HookInsertBegin // - HookWorkBegin +// +// More operation-specific interfaces may be added in future versions. type Hook interface { // IsHook is a sentinel function to check that a type is implementing Hook // on purpose and not by accident (Hook would otherwise be an empty @@ -360,6 +362,8 @@ type HookWorkBegin interface { // List of middleware interfaces that may be implemented: // - JobInsertMiddleware // - WorkerMiddleware +// +// More operation-specific interfaces may be added in future versions. type Middleware interface { // IsMiddleware is a sentinel function to check that a type is implementing // Middleware on purpose and not by accident (Middleware would otherwise be