Fix linting issues found by golangci-lint v2.0.2 #16368
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR automatically fixes several linting issues identified by golangci-lint v2.0.2 in preparation for upcoming changes. The changes primarily address code style, error message formatting, and minor refactorings across tests, parsers, and discovery modules.
Reviewed Changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| promql/parser/lex.go | Replaces explicit equality comparisons with a switch on rune |
| promql/engine.go | Uses time.Equal for time comparisons instead of "==" |
| promql/bench_test.go | Adjusts method calls to use the updated API (e.g. stor.Compact) |
| prompb/rwcommon/codec_test.go | Replaces not-nil variants and updates metric reset code |
| model/labels/labels_test.go | Changes string equality assertions to use require.Empty |
| model/histogram/float_histogram.go | Simplifies bucket iterator method call |
| discovery/scaleway/instance.go | Refactors conditional scoring using a switch-case |
| discovery/moby/docker.go | Refactors container network loop from an infinite loop to a for loop |
| discovery/marathon/marathon_test.go | Replaces empty string comparisons with require.Empty |
| discovery/manager_test.go | Updates FailNow and assertion calls for improved formatting |
| discovery/linode/linode.go | Adjusts error check logic for unauthorized errors (logically equivalent) |
| discovery/kubernetes/pod.go | Uses pod.UID instead of accessing ObjectMeta for UID |
| discovery/kubernetes/endpointslice_adaptor(_test).go | Simplifies access to endpointslice Name and Namespace fields |
| discovery/consul/consul_test.go | Switches to require.Empty for checking empty string values |
| cmd/promtool/unittest.go | Streamlines loop condition in test logic |
| cmd/promtool/main_test.go | Uses updated assertion formatting for process state checks |
| cmd/prometheus/query_log_test.go | Replaces Write and fmt.Sprintf pattern with fmt.Fprintf to write config data |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes various linting issues identified by golangci-lint v2.0.2 and re-enables linter rules that were temporarily disabled. Key changes include replacing assertions with their simpler counterparts, updating method invocations and field accesses in decoder and histogram code, and refactoring test failure messages for improved clarity.
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| prompb/rwcommon/codec_test.go | Replaced NotNilf calls with NotNil for cleaner test assertions. |
| prompb/io/prometheus/client/decoder.go | Updated method invocations and removed redundant receiver references. |
| model/labels/labels_test.go | Changed equality checks for empty strings to require.Empty. |
| model/histogram/float_histogram.go | Refactored iterator access to use the updated method call. |
| discovery/uyuni/uyuni.go | Inserted nolint comments alongside error messages for staticcheck. |
| discovery/scaleway/instance.go | Replaced if-else checks with a switch for clarity when handling IP families. |
| discovery/moby/docker.go | Simplified looping logic using a for loop with a condition. |
| discovery/marathon/marathon_test.go | Updated test assertions to use require.Empty instead of comparing with an empty string. |
| discovery/manager_test.go | Modified test failure messages in require.FailNow calls. |
| discovery/linode/linode.go | Updated error handling condition for unauthorized errors. |
| discovery/kubernetes/* | Updated label extraction and object meta accesses in pod and endpointslice adaptor code. |
| discovery/consul/consul_test.go | Replaced equality checks on empty strings with require.Empty. |
| discovery/aws/lightsail.go | Added nolint aside from region validation error message. |
| cmd/promtool/* and cmd/prometheus/* | Revised test loops and assertion failure messages for consistency. |
| .golangci.yml | Adjusted staticcheck and testifylint configurations. |
Comments suppressed due to low confidence (1)
cmd/prometheus/main_test.go:335
- The formatting parameter is provided separately from the error message; it is recommended to format the message string with the error directly for clarity.
require.Fail(t, "prometheus should be still running", "err", err)
There was a problem hiding this comment.
Pull Request Overview
This PR fixes numerous linting issues as reported by golangci-lint v2.0.2 by replacing deprecated or discouraged test assertions and updating linter configurations. The key changes include:
- Replacement of formatted nil and empty assertions with their unformatted counterparts in various test files.
- Removal of deprecated receiver field accesses and simplification of method calls.
- Updates to configuration files and comments to re-enable previously disabled linter rules.
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| prompb/rwcommon/codec_test.go | Updates test assertions for Histogram methods |
| prompb/io/prometheus/client/decoder.go | Refactors unmarshal method calls and receiver usage |
| model/labels/labels_test.go | Uses require.Empty instead of require.Equal with an empty string |
| model/histogram/float_histogram.go | Adjusts bucket iterator method call |
| discovery/* | Various updates including nolint directive additions |
| cmd/promtool/* and discovery/* tests | Updates test failure messages and assertions |
| .golangci.yml | Removes and adjusts staticcheck exclusions |
Comments suppressed due to low confidence (1)
model/histogram/float_histogram.go:1019
- Verify that the call to i.at(i.targetSchema) is correct and does not lead to infinite recursion or an undefined method invocation, since the previous version referenced i.baseBucketIterator.at.
return i.at(i.targetSchema)
There was a problem hiding this comment.
Pull Request Overview
This PR fixes various linting issues raised by golangci-lint v2.0.2 by replacing custom assertions and method calls with their simplified or more idiomatic versions.
- Update of test assertions from require.NotNilf/require.Equal to require.NotNil/require.Empty
- Refactoring of method invocations and loop constructs for clarity and compliance
- Minor configuration updates in the golangci-lint configuration file
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| prompb/rwcommon/codec_test.go | Replaces NotNilf with NotNil in histogram conversion tests |
| prompb/io/prometheus/client/decoder.go | Updates method calls from m.MetricFamily/Metric.unmarshal... to m.unmarshal... |
| model/labels/labels_test.go | Replaces Equal assertions with Empty for missing labels |
| model/histogram/float_histogram.go | Adjusts method call in bucket iterator to use i.at |
| discovery/* (uyuni, scaleway, moby, marathon, etc.) | Various linter-related updates including adding nolint comments |
| discovery/linode/linode.go | Inverts error condition to correctly check the unauthorized error |
| discovery/kubernetes/* | Updates object field accesses for pods and endpoint slices |
| discovery/consul/consul_test.go | Changes empty string assertions to require.Empty |
| discovery/aws/lightsail.go | Adds nolint comment for region requirement |
| cmd/promtool/* and cmd/prometheus/* | Small fixes in loop conditions and require.Fail messages |
| .golangci.yml | Adjusts staticcheck and testifylint rules settings |
Comments suppressed due to low confidence (4)
prompb/io/prometheus/client/decoder.go:84
- Please verify that replacing m.MetricFamily.unmarshalWithoutMetrics with m.unmarshalWithoutMetrics correctly preserves the intended behavior.
return m.unmarshalWithoutMetrics(m, m.mfData)
prompb/io/prometheus/client/decoder.go:101
- Ensure that replacing m.Metric.unmarshalWithoutLabels with m.unmarshalWithoutLabels does not change the method resolution or expected functionality.
if err := m.unmarshalWithoutLabels(m, m.mData); err != nil {
model/histogram/float_histogram.go:1019
- Confirm that calling i.at(i.targetSchema) (instead of i.baseBucketIterator.at(i.targetSchema)) achieves the same logic without side effects.
return i.at(i.targetSchema)
discovery/linode/linode.go:197
- Double-check that the inverted logic in this condition correctly handles the error case when the token lacks the required 'events:read_only' scope.
if !errors.As(err, &e) || e.Code != http.StatusUnauthorized {
|
Have you considered splitting this in two PR, one for testifylint issues, the other for staticcheck’s ? |
I don't have much time to spare for this, is it worth it? |
|
It's up to you. If reviewers can absorb this, that's fine ;) . |
There was a problem hiding this comment.
Copilot reviewed 49 out of 49 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
model/histogram/float_histogram.go:1019
- This change replaces the call to i.baseBucketIterator.at with i.at; please verify that i.at does not inadvertently recurse on itself, as this could lead to an infinite recursion. Consider ensuring that an appropriate method is being invoked.
return i.at(i.targetSchema)
78a9907 to
a60762b
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes various linting issues flagged by golangci-lint v2.0.2 by applying automated refactorings and style improvements across the codebase. Key changes include updating test assertions to use require.Empty/t.Fatalf, refactoring method calls in decoding and bucket iteration, and simplifying error formatting and loop structures.
Reviewed Changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| prompb/rwcommon/codec_test.go | Removed unnecessary formatting strings from nil assertions in tests. |
| prompb/io/prometheus/client/decoder.go | Refactored method calls and field accesses for unmarshalling functionality. |
| model/labels/labels_test.go | Replaced equality assertions with require.Empty for clarity. |
| model/histogram/float_histogram.go | Updated bucket iterator call from baseBucketIterator.at to i.at. |
| documentation/examples/remote_storage/remote_storage_adapter/opentsdb/tagvalue.go | Changed to fmt.Fprintf for more efficient formatted writing. |
| documentation/examples/remote_storage/remote_storage_adapter/main.go | Improved error printing with fmt.Fprintf instead of fmt.Fprintln(fmt.Errorf(...)). |
| discovery/uyuni/uyuni.go | Added nolint comments to suppress staticcheck warnings on error messages. |
| discovery/scaleway/instance.go | Switched from if/else to a switch statement for IP family handling. |
| discovery/moby/docker.go | Simplified container network lookup loop using a for condition. |
| discovery/marathon/marathon_test.go | Standardized test assertions and error reporting by replacing require.Equal with require.Empty and require.FailNow with t.Fatalf. |
| discovery/manager_test.go | Updated error reporting in test cases with t.Fatalf and require.Len. |
| discovery/linode/linode.go | Refactored error condition logic for unauthorized responses using errors.As. |
| discovery/kubernetes/pod.go | Simplified label value assignment for pod UID. |
| discovery/consul/consul_test.go | Replaced equality assertions with require.Empty for empty strings. |
| discovery/aws/lightsail.go | Added nolint comments to suppress staticcheck warnings. |
| cmd/promtool/unittest.go | Simplified loop structure in unit test logic. |
| cmd/promtool/main_test.go | Improved error reporting using t.Fatalf in multiple test cases. |
| cmd/prometheus/query_log_test.go | Refactored configuration file writing to use fmt.Fprintf. |
| cmd/prometheus/main_test.go | Updated process error handling by replacing require.Fail with t.Fatalf. |
| .golangci.yml | Removed multiple disabled staticcheck sub-rules and updated configuration to reflect new linting standards. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
|
Thanks for the suggestion @bwplotka. I think I will just merge now though, a bit too tired to try to rewrite that code. |
Fix linting issues found by golangci-lint v2.0.2, which was upgraded to in #16356. Most of the issues were fixed automatically via
golangci-lint run ./....I re-enable corresponding linter rules that were temporarily disabled in #16356.