Skip to content

golangci-lint: enable more linters and address linting issues#191

Merged
rhatdan merged 12 commits intoopencontainers:mainfrom
thaJeztah:update_golangci
Oct 17, 2022
Merged

golangci-lint: enable more linters and address linting issues#191
rhatdan merged 12 commits intoopencontainers:mainfrom
thaJeztah:update_golangci

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Oct 12, 2022

(more coming after this, but I have ETOOMANYBRANCHES already)

@thaJeztah thaJeztah mentioned this pull request Oct 12, 2022
Merged
@rhatdan
Copy link
Copy Markdown
Collaborator

rhatdan commented Oct 12, 2022

I am ok with almost all of these, although I am not crazy about the err:= shadow issue. I find it a lot easier to read

if err := foobar(); err != nil
}

Then
err := foobar()
if err != nil {
}

Bottom line, I think err should be handled differently then other shadowed vars. But I am willing to live with the changes.

LGTM

@thaJeztah thaJeztah changed the title golangci-lint: enable more linters and address listing issues golangci-lint: enable more linters and address linting issues Oct 13, 2022
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 13, 2022

I am not crazy about the err:= shadow issue.

Yes, I'm slightly on the fence on that one as well;

  • in most cases, it's mostly "noise"
  • ^^ that said, I'm not against doing a "once in a while" round on changing them
  • ^^^ but ... that said; I've also encountered situations where an err was masking an err that was checked in a defer(). Admitted, those situations are rare (and usually better to have a distinct name for "special" errors).

I don't think the linter itself has a config for this, but perhaps a string match exclusion would work (see https://golangci-lint.run/usage/false-positives/#exclude-issue-by-text)

@rhatdan
Copy link
Copy Markdown
Collaborator

rhatdan commented Oct 13, 2022

Tests are failing for go-fumpt?

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 13, 2022

Tests are failing for go-fumpt?

I probably need to rebase see #189 (comment)

@thaJeztah
golangci: enable govet shadow-check, remove whitespace, sort linters
e4a3457 
- The whitespace linter overlaps with gofumpt, so doesn't bring much.
- Enable govet's shadow check to check for shadowed variables.
- Add a short description to each linter.

    pkg/pwalk/pwalk_test.go:78:8: shadow: declaration of "err" shadows declaration at line 69 (govet)
                fi, err := os.CreateTemp(dir, "f-")
                    ^
    pkg/pwalkdir/pwalkdir_test.go:82:8: shadow: declaration of "err" shadows declaration at line 73 (govet)
                fi, err := os.CreateTemp(dir, "f-")
                    ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
go-selinux: fix variable naming (revive)
46e6de3 
    go-selinux/selinux_linux.go:1089:2: var-naming: don't use underscores in Go names; var exclude_paths should be excludePaths
(revive)
    go-selinux/selinux_linux.go:622:1: receiver-naming: receiver name l1 should be consistent with previous receiver name l for level
(revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
go-selinux: rename InvalidLabel to ErrInvalidLabel (revive)
f2c3ea1 
    go-selinux/selinux.go:27:2: error-naming: error var InvalidLabel should have name of the form ErrFoo (revive)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci: enable revive
e528b89 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci: enable dupword
0f62928 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci: enable various additional linters
f7275d5 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci: show all linting issues
c249d6b 
By default, output is limited to 50 issues per linter, and 3 "same"
issues. When enabling a new linter, this requires multiple runs of
the linter to get a list of all issues.

Let's assume we don't introduce "many" new issues at a time, and just
show all the issues that were found.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci: enable gosec linter
40803cc 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review October 16, 2022 20:01
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 16, 2022

Rebased, and tweaked the settings to ignore "err" for shadowing 👍 PTAL

@thaJeztah
pkg/(pwalk|pwalkdir): fix fieldalignment (govet)
c4c67f4 
    pkg/pwalk/pwalk.go:120:15: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
    type walkArgs struct {
                  ^
    pkg/pwalk/pwalk_test.go:136:18: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
        benchmarks := []struct {
                        ^
    pkg/pwalk/pwalk_test.go:146:15: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
        walkers := []struct {
                     ^
    pkg/pwalkdir/pwalkdir.go:113:15: fieldalignment: struct with 32 pointer bytes could be 24 (govet)
    type walkArgs struct {
                  ^
    pkg/pwalkdir/pwalkdir_test.go:140:18: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
        benchmarks := []struct {
                        ^
    pkg/pwalkdir/pwalkdir_test.go:150:15: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
        walkers := []struct {
                     ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 16, 2022

Oh! Had one more change staged in another branch; just pushed that one as well.

@thaJeztah
go-selinux: fix fieldalignment (govet)
3e945d5 
    go-selinux/selinux_linux.go:37:19: fieldalignment: struct with 40 pointer bytes could be 16 (govet)
    type selinuxState struct {
                      ^
    go-selinux/selinux_linux.go:46:12: fieldalignment: struct with 16 pointer bytes could be 8 (govet)
    type level struct {
               ^
    go-selinux/selinux_linux.go:56:19: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
    type defaultSECtx struct {
                      ^
    go-selinux/selinux_linux_test.go:310:13: fieldalignment: struct with 64 pointer bytes could be 56 (govet)
        tests := []struct {
                   ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
golangci-lint: enable all govet checks
ba8946a 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
rhatdan
rhatdan approved these changes Oct 17, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@kolyshkin @giuseppe @vrothberg PTAL

@rhatdan
Copy link
Copy Markdown
Collaborator

rhatdan commented Oct 17, 2022

Nice work @thaJeztah

vrothberg
vrothberg reviewed Oct 17, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan rhatdan merged commit 9832127 into opencontainers:main Oct 17, 2022
@thaJeztah thaJeztah deleted the update_golangci branch October 17, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan approved these changes

@kolyshkin kolyshkin Awaiting requested review from kolyshkin kolyshkin is a code owner

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@runcom runcom Awaiting requested review from runcom runcom is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @rhatdan @vrothberg