golangci-lint spring cleaning and bump #997

kolyshkin · 2022-04-10T01:21:51Z

This

fixes (or suppresses) some golangci-lint related warnings;
simplifies golangci-lint config for the sake of future maintainers;
bumps golangci-lint to v1.45.2.
adds a GHA CI job (a dup of one in cirrus-ci but separate, fast, and can annotate PRs).

TODO (not in this PR, just ideas)

maybe enable more linters;
maybe a separate lint job for new code (PRs) only and stricter rules (extra linters);

⚠️ Please review commit by commit, and see commit messages for all the gory details.

Commit a970d99 contains retract directive, which is only supported since Go 1.16. Bump go directive to match that. Also, the current code can no longer be compiled with Go 1.15 anyway (for example, because of filepath.WalkDir usage which is added in Go 1.16). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Error checking is important. In these two cases, though, we don't have a way to return an error, so make it explicit that we ignore the error. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Where we can check the error, do it. Where we can not, ignore it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

> libimage/manifests/manifests.go:408:3: S1033: unnecessary guard around call to delete (gosimple) > if _, needToDelete := l.instances[instanceDigest]; needToDelete { > ^ Indeed, we can just call delete right away. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fix a couple of warnings like this one: > pkg/formats/templates.go:23:14: SA1019: strings.Title is deprecated: > The rule Title uses for word boundaries does not handle Unicode > punctuation properly. Use golang.org/x/text/cases instead. > (staticcheck) This is caused by the fact that Go 1.18 strings package made a (somewhat strange) decision to deprecate strings.Title. The suggestion to replace it with a new package is a bit too much. Silence the warning instead, as we are pretty sure this function works for us. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

gofumpt is a stricter version of gofmt, basically making the code more readable, and fixing the gocritic's octalLiterar warnings like this one: pkg/util/util_supported.go:26:17: octalLiteral: use new octal literal style, 0o722 (gocritic) return (perm & 0722) == 0700 ^ Generated by gofumpt -w . Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This one: pkg/manifests/manifests.go:75:1: paramTypeCombine: func(manifestDigest digest.Digest, manifestSize int64, manifestType, osName, architecture, osVersion string, osFeatures []string, variant string, features []string, annotations []string) error could be replaced with func(manifestDigest digest.Digest, manifestSize int64, manifestType, osName, architecture, osVersion string, osFeatures []string, variant string, features, annotations []string) error (gocritic) func (l *list) AddInstance(manifestDigest digest.Digest, manifestSize int64, manifestType, osName, architecture, osVersion string, osFeatures []string, variant string, features []string, annotations []string) error { ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This one: libimage/manifests/manifests.go:387:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic) } else { ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Like this one: pkg/manifests/manifests.go:449:4: importShadow: shadow of imported from 'github.com/containers/image/v5/manifest' package 'manifest' (gocritic) manifest, err := json.Marshal(&l.oci) ^ First, this is not always a "manifest" (sometimes it's OCI image index). Second, using two variables (manifest and manifestBytes) is not needed. Finally, rename manifest to res (stands for result). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This removes the questionable Sys().(*syscall.Stat_t) typecast. OTOH we have to handle EINTR, so it's a tad more complicated than it should be. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This one: libnetwork/netavark/ipam_test.go:45:22: Error return value is not checked (errcheck) networkInterface = libpodNet.(*netavarkNetwork) ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Greatly simplify the golangci-lint config. 1.1. Remove the skip-dir list (of excluded directories), since as all the issues are now fixed). 1.2. Instead of enabling all the linters and then disabling many of them, go with the default set of enabled linters, and then add some (currently just gocritic and dupl). This will hopefully make upgrading golangci-lint easier, as it maintains a sensitive set of linters enabled by default (and enabling all linters by default means we will enable some new and unknown linters when we bump golangci-lint version). 1.3. Remove the gocritic list of enabled linters, use a default one. The motivation is similar -- it is hard to maintain/upgrade otherwise. 1.4. Remove the "check-blank: true" option from the errcheck linter, as we often use _ = fn() type of assignment to denote that we want to ignore the error. 2. Simplify golangci-lint installation from the Makefile. 3. Bump golangci-lint to 1.45.2. NOTE that if you are using golangci-lint locally, you have to remove it from the build directory (i.e. rm -f build/golangci-lint) before running "make validate"). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

vrothberg

Thanks!

LGTM other than the nit.

@containers/podman-maintainers PTAL

.golangci.yml

libnetwork/netavark/ipam.go

libnetwork/network/interface.go

When the "nolint:gocritic" hint is removed, gocritic says: pkg/timetype/timestamp.go:37:2: ifElseChain: rewrite if-else to switch statement (gocritic) if strings.Contains(value, ".") { ^ Indeed, it makes sense to do so. Rewrite the code, remove the nolint:golint. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Those were added by commit 39a8401, but are apparently no longer needed. Reported-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2022-04-11T23:59:17Z

No longer a draft. I took a liberty to add a GHA CI job for golangci-lint (while it adds some duplication, it is very fast and whoever opens a PR will know if there are issues in a minute, not half an hour).

This is currently similar to what make validate does in .cirrus.yml, but let's have it as a separate job since it it faster. Let's keep the same job in Cirrus CI for now. Note we do not ping Go version, but rather depend on action/setup-go to know which one is latest. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

And fix the found warnings: pkg/config/systemd.go:61:3: S1023: redundant `return` statement (gosimple) return ^ pkg/config/systemd.go:85:3: S1023: redundant `return` statement (gosimple) return ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Luap99 · 2022-04-12T08:55:12Z

.github/workflows/validate.yml

+  lint:
+    runs-on: ubuntu-20.04
+    steps:
+    - uses: actions/checkout@v3
+    - uses: actions/setup-go@v3
+      with:
+        go-version: 1.x # latest stable
+    - name: install deps
+      run: |
+        sudo apt-get -qq update
+        sudo apt-get -qq install libseccomp-dev libdevmapper-dev
+    - name: lint
+      uses: golangci/golangci-lint-action@v3
+      with:
+        version: "${{ env.LINT_VERSION }}"
+    - name: validate seccomp
+      run: ./tools/validate_seccomp.sh ./pkg/seccomp


We already run this as part of the normal cirrus job so this is redundant.

Yes, I am aware of that (this is where I copied the code from). I guess I did that since I was thinking about removing this step (running the linter) from the cirrus job (but have not done it yet).

Lets do this in a separate PR

rhatdan · 2022-04-14T11:12:30Z

/approve
/lgtm

openshift-ci · 2022-04-14T11:12:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg · 2022-04-14T11:37:11Z

Thanks for the cleanups, @kolyshkin !

kolyshkin added 12 commits April 9, 2022 14:51

Silence errcheck linter warnings

ccd2562

Error checking is important. In these two cases, though, we don't have a way to return an error, so make it explicit that we ignore the error. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

*_test.go: fix or suppress errcheck warnings

0039f7f

Where we can check the error, do it. Where we can not, ignore it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libimage/manifests: fix ifelse gocritic warning

15801ce

This one: libimage/manifests/manifests.go:387:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic) } else { ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libimage: suppress appendAssign gocritic warning

7a85de1

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libnetwork/cni: use unix pkg to get file time

bdf2f90

This removes the questionable Sys().(*syscall.Stat_t) typecast. OTOH we have to handle EINTR, so it's a tad more complicated than it should be. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libnetwork/netavark:: suppress errcheck warn

98ddeca

This one: libnetwork/netavark/ipam_test.go:45:22: Error return value is not checked (errcheck) networkInterface = libpodNet.(*netavarkNetwork) ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

openshift-ci bot added the do-not-merge/work-in-progress label Apr 10, 2022

vrothberg reviewed Apr 11, 2022

View reviewed changes

.golangci.yml Show resolved Hide resolved

Luap99 reviewed Apr 11, 2022

View reviewed changes

libnetwork/netavark/ipam.go Outdated Show resolved Hide resolved

Luap99 reviewed Apr 11, 2022

View reviewed changes

libnetwork/network/interface.go Outdated Show resolved Hide resolved

kolyshkin changed the title ~~Golangci lint spring cleaning and bump~~ golangci-lint spring cleaning and bump Apr 11, 2022

kolyshkin added 2 commits April 11, 2022 15:01

libnetwork: rm nolint:golint hints

b408d78

Those were added by commit 39a8401, but are apparently no longer needed. Reported-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the golangci-lint branch 9 times, most recently from feafbd0 to 88414bb Compare April 11, 2022 22:58

kolyshkin requested a review from vrothberg April 11, 2022 23:24

kolyshkin requested a review from Luap99 April 11, 2022 23:24

kolyshkin marked this pull request as ready for review April 11, 2022 23:58

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 11, 2022

kolyshkin force-pushed the golangci-lint branch from 1267395 to 0a93c9c Compare April 12, 2022 00:09

kolyshkin added 2 commits April 11, 2022 17:10

.golangci.yml: add systemd build tag

0e669c0

And fix the found warnings: pkg/config/systemd.go:61:3: S1023: redundant `return` statement (gosimple) return ^ pkg/config/systemd.go:85:3: S1023: redundant `return` statement (gosimple) return ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the golangci-lint branch from 0a93c9c to 0e669c0 Compare April 12, 2022 00:10

Luap99 reviewed Apr 12, 2022

View reviewed changes

openshift-ci bot assigned rhatdan Apr 14, 2022

openshift-ci bot added the lgtm label Apr 14, 2022

openshift-ci bot added the approved label Apr 14, 2022

openshift-merge-robot merged commit 5215683 into containers:main Apr 14, 2022

kolyshkin mentioned this pull request May 17, 2022

cirrus-ci: rm make validate #1041

Merged

kolyshkin mentioned this pull request Mar 26, 2025

Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

golangci-lint spring cleaning and bump #997

golangci-lint spring cleaning and bump #997

Uh oh!

kolyshkin commented Apr 10, 2022 •

edited

Loading

Uh oh!

vrothberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Apr 11, 2022

Uh oh!

Luap99 Apr 12, 2022

Uh oh!

kolyshkin Apr 13, 2022

Uh oh!

rhatdan Apr 14, 2022

Uh oh!

rhatdan commented Apr 14, 2022

Uh oh!

openshift-ci bot commented Apr 14, 2022

Uh oh!

vrothberg commented Apr 14, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

golangci-lint spring cleaning and bump #997

golangci-lint spring cleaning and bump #997

Uh oh!

Conversation

kolyshkin commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Apr 11, 2022

Uh oh!

Luap99 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

kolyshkin Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

rhatdan Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 14, 2022

Uh oh!

openshift-ci bot commented Apr 14, 2022

Uh oh!

vrothberg commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Apr 10, 2022 •

edited

Loading

vrothberg commented Apr 14, 2022 •

edited

Loading