chore(deps): Bump golangci-lint from v1.64.5 to v2.0.2 #16683

zak-pawel · 2025-03-24T23:15:54Z

Summary

More info about major version update: https://golangci-lint.run/product/changelog/#201

In this PR:

I migrated the .golangci-lint config from v1 to v2 using: golangci-lint migrate: https://golangci-lint.run/product/migration-guide/.
I supplemented the resulting config with removed comments and made it as similar as possible to https://github.com/golangci/golangci-lint/blob/main/.golangci.reference.yml to facilitate future diffs.
The linters staticcheck, stylecheck, and gosimple have been merged into a single linter (staticcheck). A lot of new issues were detected by staticcheck, but I'm not addressing them in this PR. Instead, I disabled the newly appearing checks - we can consider handling them in the future.
Other minor config changes were made to align with the new schema.
I fixed new findings:

config/config_test.go:906:4                                                   testifylint  len: use require.Len
internal/internal_test.go:88:2                                                testifylint  empty: use require.Empty
internal/templating/engine_test.go:22:2                                       testifylint  empty: use require.Empty
migrations/global_agent/migration_test.go:91:4                                testifylint  useless-assert: meaningless assertion
plugins/common/jolokia2/gatherer_test.go:99:3                                 testifylint  len: use require.Len
plugins/inputs/amd_rocm_smi/amd_rocm_smi_test.go:32:2                         testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/amd_rocm_smi/amd_rocm_smi_test.go:51:2                         testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/amd_rocm_smi/amd_rocm_smi_test.go:70:2                         testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/clickhouse/clickhouse_test.go:18:2                             testifylint  empty: use require.Empty
plugins/inputs/dpdk/dpdk_connector_test.go:120:4                              testifylint  len: use require.Len
plugins/inputs/dpdk/dpdk_test.go:34:3                                         testifylint  empty: use require.Empty
plugins/inputs/dpdk/dpdk_utils_test.go:88:3                                   testifylint  empty: use require.Empty
plugins/inputs/intel_baseband/utils_test.go:62:4                              testifylint  empty: use require.Empty
plugins/inputs/intel_baseband/utils_test.go:80:4                              testifylint  empty: use require.Empty
plugins/inputs/intel_baseband/utils_test.go:96:4                              testifylint  empty: use require.Empty
plugins/inputs/intel_dlb/intel_dlb_test.go:29:3                               testifylint  empty: use require.Empty
plugins/inputs/intel_powerstat/intel_powerstat_test.go:692:2                  testifylint  empty: use require.NotEmpty
plugins/inputs/intel_rdt/intel_rdt_test.go:64:2                               testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/intel_rdt_test.go:79:2                               testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/publisher_test.go:60:3                               testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/publisher_test.go:78:3                               testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/publisher_test.go:97:3                               testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/publisher_test.go:190:4                              testifylint  empty: use require.Empty
plugins/inputs/intel_rdt/publisher_test.go:191:4                              testifylint  empty: use require.Empty
plugins/inputs/internal/internal_test.go:110:4                                testifylint  useless-assert: meaningless assertion
plugins/inputs/modbus/modbus_test.go:473:2                                    testifylint  len: use require.Len
plugins/inputs/modbus/modbus_test.go:479:3                                    testifylint  len: use require.Lenf
plugins/inputs/nvidia_smi/nvidia_smi_test.go:82:2                             testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/nvidia_smi/nvidia_smi_test.go:101:2                            testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/nvidia_smi/nvidia_smi_test.go:120:2                            testifylint  error-is-as: use require.NotErrorAs
plugins/inputs/prometheus/kubernetes_test.go:213:2                            testifylint  empty: use require.Empty
plugins/inputs/stackdriver/stackdriver_test.go:1211:4                         testifylint  len: use require.Len
plugins/inputs/statsd/statsd_test.go:1966:2                                   testifylint  empty: use require.Emptyf
plugins/inputs/varnish/varnish_test.go:613:2                                  testifylint  len: use require.Len
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:174:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:176:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:213:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:215:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:250:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:252:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:287:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:289:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:352:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go:354:3  testifylint  formatter: failure message is not a format string, use msgAndArgs instead
plugins/outputs/amqp/amqp_test.go:67:5                                        testifylint  empty: use require.Empty
plugins/outputs/cloudwatch/cloudwatch_test.go:35:3                            testifylint  len: use require.Len
plugins/outputs/elasticsearch/elasticsearch_test.go:591:3                     testifylint  empty: use require.Empty
plugins/outputs/influxdb/http_test.go:819:5                                   testifylint  empty: use require.Empty
plugins/outputs/influxdb/http_test.go:923:5                                   testifylint  empty: use require.Empty
plugins/outputs/kinesis/kinesis_test.go:71:2                                  testifylint  empty: use require.Empty
plugins/outputs/kinesis/kinesis_test.go:523:2                                 testifylint  len: use require.Lenf
plugins/outputs/kinesis/kinesis_test.go:538:3                                 testifylint  len: use require.Lenf
plugins/outputs/stackdriver/stackdriver_test.go:217:4                         testifylint  useless-assert: meaningless assertion
plugins/outputs/stackdriver/stackdriver_test.go:293:4                         testifylint  useless-assert: meaningless assertion
plugins/outputs/stackdriver/stackdriver_test.go:353:4                         testifylint  useless-assert: meaningless assertion
plugins/outputs/timestream/timestream_test.go:1268:2                          testifylint  len: use require.Len
plugins/outputs/wavefront/wavefront_test.go:403:2                             testifylint  empty: use require.Empty
plugins/outputs/zabbix/autoregister_test.go:71:2                              testifylint  empty: use require.Empty
plugins/outputs/zabbix/autoregister_test.go:72:2                              testifylint  empty: use require.Empty
plugins/parsers/collectd/parser_test.go:316:2                                 testifylint  len: use require.Len
plugins/parsers/influx/influx_upstream/parser_test.go:624:4                   testifylint  len: use require.Len
plugins/parsers/influx/influx_upstream/parser_test.go:757:4                   testifylint  len: use require.Len
plugins/parsers/influx/influx_upstream/parser_test.go:967:4                   testifylint  len: use require.Len
plugins/parsers/influx/parser_test.go:594:4                                   testifylint  len: use require.Len
plugins/parsers/influx/parser_test.go:833:4                                   testifylint  len: use require.Len
plugins/parsers/influx/parser_test.go:934:4                                   testifylint  len: use require.Len
plugins/parsers/json_v2/parser_test.go:94:5                                   testifylint  len: use require.Len
plugins/parsers/nagios/parser_test.go:83:2                                    testifylint  len: use require.Len
plugins/serializers/graphite/graphite_test.go:369:2                           testifylint  empty: use require.Empty
plugins/serializers/graphite/graphite_test.go:393:2                           testifylint  empty: use require.Empty

Checklist

No AI generated code was used in this PR

srebhan

Thanks for the bump @zak-pawel! Some comments in the code. I especially wonder why you move from require.Failf to require.Fail(t, fmt.Sprintf(...))... I prefer the former.

.golangci.yml

srebhan · 2025-03-25T09:21:06Z

.golangci.yml

+        # Poorly chosen identifier.
+        # https://staticcheck.dev/docs/checks/#ST1003
+        - -ST1003
+        # Incorrectly formatted error string.
+        # https://staticcheck.dev/docs/checks/#ST1005
+        - -ST1005
+        # Should use constants for HTTP error codes, not magic numbers.
+        # https://staticcheck.dev/docs/checks/#ST1013
+        - -ST1013
+        # Use consistent method receiver names.
+        # https://staticcheck.dev/docs/checks/#ST1016
+        - -ST1016
+        # Don't use Yoda conditions.
+        # https://staticcheck.dev/docs/checks/#ST1017
+        - -ST1017
+        # The documentation of an exported function should start with the function's name.
+        # https://staticcheck.dev/docs/checks/#ST1020
+        - -ST1020
+        # The documentation of an exported type should start with type's name.
+        # https://staticcheck.dev/docs/checks/#ST1021
+        - -ST1021
+        # The documentation of an exported variable or constant should start with variable's name.
+        # https://staticcheck.dev/docs/checks/#ST1022
+        - -ST1022
+        # Apply De Morgan's law.
+        # https://staticcheck.dev/docs/checks/#QF1001
+        - -QF1001
+        # Convert untagged switch to tagged switch.
+        # https://staticcheck.dev/docs/checks/#QF1002
+        - -QF1002
+        # Convert if/else-if chain to tagged switch.
+        # https://staticcheck.dev/docs/checks/#QF1003
+        - -QF1003
+        # Use 'strings.ReplaceAll' instead of 'strings.Replace' with 'n == -1'.
+        # https://staticcheck.dev/docs/checks/#QF1004
+        - -QF1004
+        # Lift 'if'+'break' into loop condition.
+        # https://staticcheck.dev/docs/checks/#QF1006
+        - -QF1006
+        # Merge conditional assignment into variable declaration.
+        # https://staticcheck.dev/docs/checks/#QF1007
+        - -QF1007
+        # Omit embedded fields from selector expression.
+        # https://staticcheck.dev/docs/checks/#QF1008
+        - -QF1008
+        # Use 'time.Time.Equal' instead of '==' operator.
+        # https://staticcheck.dev/docs/checks/#QF1009
+        - -QF1009


Which one of those are the ones that we should work on? All? Like if I want to do some linter work, which ones should I remove and work on the fallout? Could you please split this into two sections the "need to stay disabled" as we don't want them to run e.g. because we don't like what they enforce, and the "should be fixed" ones which we need to remove later.

At the moment, all new findings from staticcheck (so merged: staticcheck, stylecheck, and gosimple) are disabled using a - sign before the check name (e.g., -ST1017 means that the check for Don't use Yoda conditions. is disabled).

My plan was to create a new issue for each of these disabled checks, where I would describe its functionality, list all findings, and discuss whether we want to enable it or not. Just like always :) I wanted to do this after merging this PR.

I could have explained this a bit more clearly in the PR description.

OK, so all of them need to be tackled later? Like I was under the impression that we don't want to document exported function, at least not in plugins/... Therefore, I thought e.g. ST1020 would be a candidate that stays disabled.. Am I mistaken here?

Most likely, but I would continue this discussion in separate issues.

srebhan · 2025-03-25T09:25:00Z

migrations/global_agent/migration_test.go

-			require.Positive(t, n, "expected migration application but none applied")
+			require.NotZero(t, n, "expected migration application but none applied")


Hmmm that's something different, as e.g. -1 would error before but not now. However, I'm not sure if ApplyMigrations returns an unsigned integer but in any case I would prefer the former as it is more informative and actually what we expect.

I tried to workaround false positive finding here (useless-assert: meaningless assertion).
Since this is uint64, it will never be less than 0.

But if you want, I can revert it to the previous state and add //nolint with a false-positive description.
Let me know.

I do understand but with Positive you don't need to know if the result is signed or not, you do directly understand what we want. For NotZero you need to know this is uint and cannot be negative. I also veto that useless-assert: meaningless assertion is correct here! This is a bug!

Back to Positive with //nolint. I will try to isolate this and fill the bug.

plugins/aggregators/histogram/histogram_test.go

srebhan · 2025-03-25T09:30:56Z

plugins/inputs/mock/mock_test.go

-			require.Failf(t, "unexpected field %q", k)
+			require.Fail(t, fmt.Sprintf("Unexpected field %q", k))


I thought you might notice that 😜
I'll respond here once, but this applies to all comments related to require.Failxxx().

If I simply use require.Failf(), then (in case of failure), the result will look like this:

Not very pretty, right? It's all because of the function signatures:

The versions with f (FailNowf and Failf) require that the optional formatted string be the second string argument, not the first. The first one is mandatory and serves as the failureMessage.

There are two options here:

Format the string using fmt.Sprintf() and pass it as failureMessage.

Provide a completely different string as failureMessage (but which one?), while passing the data needed for formatting as the third and fourth arguments.

As for preventing deferred functions from running: t.FailNow() is called in every require.XXX function when the condition is not met. And I guess defer still gets executed 😉

OK, in this case I opt for

require.Failf(t, "unexpected field", k)

I was not aware of this stupid behavior... 🤦

OK, in this case I opt for

require.Failf(t, "unexpected field", k)

I was not aware of this stupid behavior... 🤦

@srebhan

Output from version you're suggesting doesn't look the best...

All require.Failxxx() comments addressed: 357e548

srebhan · 2025-03-25T09:34:33Z

plugins/inputs/win_perf_counters/win_perf_counters_integration_test.go

-		require.FailNow(t, "no results returned from the counterPath: %v", len(hostCounters.counters))
+		require.FailNow(t, fmt.Sprintf("No results returned from the counterPath: %v", len(hostCounters.counters)))
 	} else if len(hostCounters.counters) > 1 {
-		require.FailNow(t, "too many results returned from the counterPath: %v", len(hostCounters.counters))
+		require.FailNow(t, fmt.Sprintf("Too many results returned from the counterPath: %v", len(hostCounters.counters)))


require.FailNowf! Anyway, this will prevent any deferred function to run IIRC. Shouldn't we simply test with
require.Len(t, hostCounters.counters, 1)? Same for the other tests below.

srebhan · 2025-03-25T09:35:51Z

plugins/outputs/stackdriver/stackdriver_test.go

@@ -214,7 +214,7 @@ func TestWriteTagsAsResourceLabels(t *testing.T) {
 		case "test_cpu_value/unknown":
 			require.Equal(t, "cpu", ts.Resource.Labels["job_name"])
 		default:
-			require.False(t, true, "Unknown metric type")
+			require.Fail(t, fmt.Sprintf("Unknown metric type: %v", ts.Metric.Type))


require.Failf. Same for all other instances!

srebhan · 2025-03-25T09:36:38Z

testutil/accumulator.go

@@ -399,8 +399,7 @@ func (a *Accumulator) AssertContainsTaggedFields(
 			t.Log("measurement", p.Measurement, "tags", p.Tags, "fields", p.Fields)
 		}
 	}
-	msg := fmt.Sprintf("unknown measurement %q with tags %v", measurement, tags)
-	require.Fail(t, msg)
+	require.Fail(t, fmt.Sprintf("Unknown measurement %q with tags %v", measurement, tags))


Suggested change

require.Fail(t, fmt.Sprintf("Unknown measurement %q with tags %v", measurement, tags))

require.Failf(t, "Unknown measurement %q with tags %v", measurement, tags)

srebhan · 2025-03-25T09:37:06Z

testutil/accumulator.go

-				"found measurement %s with tagged fields (tags %v) which should not be there",
-				measurement, tags)
-			require.Fail(t, msg)
+			require.Fail(t, fmt.Sprintf("Found measurement %s with tagged fields (tags %v) which should not be there", measurement, tags))


Please use require.Failf wherever possible!

telegraf-tiger · 2025-03-25T22:56:01Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Awesome @zak-pawel! Thanks a lot!

skartikey

@zak-pawel Great work on this PR!

(cherry picked from commit 080e9a1)

chore(deps): Bump golangci-lint from v1.64.5 to v2.0.1

6e8a9fb

telegraf-tiger bot added the chore label Mar 24, 2025

valid install command

33ba4eb

zak-pawel requested review from srebhan and mstrandboge March 24, 2025 23:45

zak-pawel assigned srebhan and mstrandboge Mar 24, 2025

zak-pawel added the linter label Mar 24, 2025

zak-pawel marked this pull request as ready for review March 24, 2025 23:45

zak-pawel requested a review from skartikey March 25, 2025 00:04

zak-pawel assigned skartikey Mar 25, 2025

srebhan reviewed Mar 25, 2025

View reviewed changes

srebhan unassigned skartikey and mstrandboge Mar 25, 2025

zak-pawel added 3 commits March 25, 2025 16:33

//nolint false-positive

b9a8894

require.Failxxx()

357e548

v2.0.2

c16f99d

zak-pawel changed the title ~~chore(deps): Bump golangci-lint from v1.64.5 to v2.0.1~~ chore(deps): Bump golangci-lint from v1.64.5 to v2.0.2 Mar 25, 2025

srebhan approved these changes Mar 27, 2025

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Mar 27, 2025

srebhan assigned skartikey and mstrandboge and unassigned srebhan Mar 27, 2025

skartikey unassigned mstrandboge Mar 27, 2025

skartikey approved these changes Mar 31, 2025

View reviewed changes

skartikey merged commit 080e9a1 into influxdata:master Mar 31, 2025
29 checks passed

github-actions bot added this to the v1.34.2 milestone Mar 31, 2025

zak-pawel mentioned this pull request Apr 2, 2025

Linter: staticcheck, Rule: ST1003 - Poorly chosen identifier. Should we enable it? #16722

Closed

srebhan pushed a commit that referenced this pull request Apr 14, 2025

chore(deps): Bump golangci-lint from v1.64.5 to v2.0.2 (#16683)

bc05c06

(cherry picked from commit 080e9a1)

		require.Positive(t, n, "expected migration application but none applied")
		require.NotZero(t, n, "expected migration application but none applied")

		require.Failf(t, "unexpected field %q", k)
		require.Fail(t, fmt.Sprintf("Unexpected field %q", k))

	require.Fail(t, fmt.Sprintf("Unknown measurement %q with tags %v", measurement, tags))
	require.Failf(t, "Unknown measurement %q with tags %v", measurement, tags)

chore(deps): Bump golangci-lint from v1.64.5 to v2.0.2 #16683

chore(deps): Bump golangci-lint from v1.64.5 to v2.0.2 #16683

Uh oh!

Conversation

zak-pawel commented Mar 24, 2025 • edited by srebhan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zak-pawel Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

telegraf-tiger bot commented Mar 25, 2025

Artifact URLs

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

skartikey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zak-pawel commented Mar 24, 2025 •

edited by srebhan

Loading

zak-pawel Mar 25, 2025 •

edited

Loading