Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: golangci-lint bump, fixes, and cleanups #3615

Merged
merged 7 commits into from
Jan 23, 2025
Merged

Conversation

kolyshkin
Copy link
Contributor

Please see individual commits for details.

Fix these two (reported by golangci-lint v1.61.0):

resctrl/utils.go:180:15: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
		return nil, fmt.Errorf(noContainerNameError)
		            ^
resctrl/utils.go:214:14: SA1006: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
		return "", fmt.Errorf(noPidsPassedError)
		           ^

Signed-off-by: Kir Kolyshkin <[email protected]>
Fix the following errcheck warning (reported by golangci-lint v1.61.0):

container/libcontainer/handler.go:741:13: Error return value of `fmt.Sscanf` is not checked (errcheck)
		fmt.Sscanf(fs[4], "%X:%X", &rx, &tx)
		          ^

While at it, simplify instantiation of the adjacent variables.

Signed-off-by: Kir Kolyshkin <[email protected]>
As reported by golangci-lint v1.61.0:

internal/storage/bigquery/client/client.go:231:21: printf: non-constant format string in call to fmt.Errorf (govet)
		return fmt.Errorf(errstr)
		                  ^

Signed-off-by: Kir Kolyshkin <[email protected]>
Fix a few warnings like this one (currently masked by config):

utils/cpuload/netlink/netlink.go:50:14: Error return value of `binary.Write` is not checked (errcheck)
	binary.Write(w, Endian, m.GenHeader)
	            ^

Signed-off-by: Kir Kolyshkin <[email protected]>
With commit 300242c, my intention was to use whatever version of
golangci-lint is available locally, and if not (which is usually the
case in CI), install the version recommended by the Makefile.

The code was changed since, and currently has two issues wrt how
golangci-lint is installed:

1. golangci-lint prints version without v prefix (e.g.
   "... version 1.23.45 ..."), while grep looks for v1.23.45.
   As a result, golangci-lint is *always* reinstalled.

2. If a newer golangci-lint version is installed locally, running
   "make lint" will downgrade it. This is not what most developers
   would expect.

Let's fix both. A version in the Makefile is now minimally
required version, and no attempt to downgrade is made.

Fixes: ea3afbc ("Build changes for golang 1.18")
Signed-off-by: Kir Kolyshkin <[email protected]>
While at it, declutter .golangci.yml by removing most exceptions.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Oh well, can I please have a free a CI run here? 😆

This is not used and was not used for quite some time, so can be safely
removed.

If there's a need to check for capitalized error messages, we can setup
stylecheck linter with enabled ST1005 (see
https://staticcheck.io/docs/configuration/options/#checks).

I ran this checker just for fun, and it gives 20+ warnings for ST1005
only, so I'm not enabling it here.

For completeness, here's how a warning from the removed script looks
like:

> Incorrect error format in file ./client/client.go: return fmt.Errorf("Status code is not OK: %v (%s)", resp.StatusCode, resp.Status)

And here's the same warning from staticcheck:

> client/client.go:214:10: ST1005: error strings should not be capitalized (stylecheck)
> 		return fmt.Errorf("Status code is not OK: %v (%s)", resp.StatusCode, resp.Status)
> 		       ^

To me, it makes sense to not capitalize error messages (and fix those
that are capitalized), but it's up to maintainers to decide.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin mentioned this pull request Oct 26, 2024
@iwankgb
Copy link
Collaborator

iwankgb commented Oct 26, 2024

/ok-to-test

@kolyshkin
Copy link
Contributor Author

@bobbypage ptal

@cwangVT
Copy link
Collaborator

cwangVT commented Dec 4, 2024

/test pull-cadvisor-e2e

@dims
Copy link
Collaborator

dims commented Jan 23, 2025

/approve
/lgtm

@dims dims merged commit affc839 into google:master Jan 23, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants