Introduce Basic Linter

[sam-heilbron]

Description

Introduce a linter, inspired by https://github.com/solo-io/gloo-mesh-enterprise/blob/main/.golangci.yaml

Context

Introducing a linter will always be a large lift, that won't be completed in a single PR. I aim to introduce the structure of the lint logic, with minimal requirements. Hopefully this will make it easier for future devs to add incremental enhancements which overall improve the health of the codebase.

In this PR I only introduce the ginkgo linter, and then I don't even make tests required as part of CI. In essence, this does nothing. However I do it for the following reasons:

• Devs can now run make analyze locally and fix tests lint issues
• We will have the lint config in our CI pipeline that other devs can expand

Auto-Fix Challenges

The ginkgolinter has auto-fix logic:

ginkgolinter -fix ./...

However, when I run that locally, I get a panic:

panic: runtime error: index out of range [0] with length 0

goroutine 23146 [running]:
github.com/nunnatsa/ginkgolinter.checkMatchErrorAssertion(0xc008a26a40?, 0xbe405e9?)
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:408 +0xaf
github.com/nunnatsa/ginkgolinter.doCheckMatchError(0xc001fae0d0, 0xc008a26a80, 0xc008a26a40, {0x13d30c0, 0xc008a269c0}, {0x13d41d0?, 0x15bfc00?}, {0xc00502c380, 0x62})
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:375 +0x279
github.com/nunnatsa/ginkgolinter.checkMatchError(0x13d30c0?, 0x13d30c0?, {0x13d30c0?, 0xc008a269c0?}, {0x13d41d0?, 0x15bfc00?}, {0xc00502c380?, 0x0?})
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:340 +0x65
github.com/nunnatsa/ginkgolinter.checkExpression(0xc001fae0d0, {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, 0xc008a26a80, ...)
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:318 +0x53f
github.com/nunnatsa/ginkgolinter.(*ginkgoLinter).run.func1({0x13d20b0?, 0xc008a22490})
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:250 +0x308
go/ast.inspector.Visit(0xc008be6080, {0x13d20b0?, 0xc008a22490?})
        /Users/samheilbron/.go/src/go/ast/walk.go:386 +0x2b
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d20b0?, 0xc008a22490?})
        /Users/samheilbron/.go/src/go/ast/walk.go:51 +0x5c
go/ast.walkStmtList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:32
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2498?, 0xc008a24840?})
        /Users/samheilbron/.go/src/go/ast/walk.go:234 +0x1f13
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2448?, 0xc008a22580?})
        /Users/samheilbron/.go/src/go/ast/walk.go:99 +0x245
go/ast.walkExprList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:26
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d25d8?, 0xc008a26dc0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:144 +0x2125
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2510?, 0xc008a1e4b0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:111 +0x385
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d25d8?, 0xc008a26e40?})
        /Users/samheilbron/.go/src/go/ast/walk.go:143 +0x6f9
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d20b0?, 0xc008a225c0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:206 +0xd16
go/ast.walkStmtList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:32
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2498?, 0xc008a248d0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:234 +0x1f13
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2448?, 0xc008a225d0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:99 +0x245
go/ast.walkExprList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:26
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d25d8?, 0xc008a26e80?})
        /Users/samheilbron/.go/src/go/ast/walk.go:144 +0x2125
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d20b0?, 0xc008a225f0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:206 +0xd16
go/ast.walkStmtList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:32
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2498?, 0xc008a24900?})
        /Users/samheilbron/.go/src/go/ast/walk.go:234 +0x1f13
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2448?, 0xc008a22600?})
        /Users/samheilbron/.go/src/go/ast/walk.go:99 +0x245
go/ast.walkExprList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:26
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d25d8?, 0xc008a26f00?})
        /Users/samheilbron/.go/src/go/ast/walk.go:144 +0x2125
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d20b0?, 0xc008a22620?})
        /Users/samheilbron/.go/src/go/ast/walk.go:206 +0xd16
go/ast.walkStmtList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:32
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2498?, 0xc008a25770?})
        /Users/samheilbron/.go/src/go/ast/walk.go:234 +0x1f13
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2448?, 0xc008a23440?})
        /Users/samheilbron/.go/src/go/ast/walk.go:99 +0x245
go/ast.walkExprList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:26
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d25d8?, 0xc008a61a00?})
        /Users/samheilbron/.go/src/go/ast/walk.go:144 +0x2125
go/ast.walkExprList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:26
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2088?, 0xc008a380f0?})
        /Users/samheilbron/.go/src/go/ast/walk.go:318 +0x1ccd
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2ad8?, 0xc008a61a40?})
        /Users/samheilbron/.go/src/go/ast/walk.go:344 +0x1b85
go/ast.walkDeclList(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:38
go/ast.Walk({0x13d0e40?, 0xc008be6080?}, {0x13d2060?, 0xc008a68000?})
        /Users/samheilbron/.go/src/go/ast/walk.go:366 +0x1b05
go/ast.Inspect(...)
        /Users/samheilbron/.go/src/go/ast/walk.go:397
github.com/nunnatsa/ginkgolinter.(*ginkgoLinter).run(0xc000116440, 0xc001fae0d0)
        /Users/samheilbron/go/pkg/mod/github.com/nunnatsa/[email protected]/ginkgo_linter.go:193 +0x3a5
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc008983220)
        /Users/samheilbron/go/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:775 +0x9f7
sync.(*Once).doSlow(0xc0002286e0?, 0xc008cc4ca8?)
        /Users/samheilbron/.go/src/sync/once.go:74 +0xbf
sync.(*Once).Do(...)
        /Users/samheilbron/.go/src/sync/once.go:65
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(...)
        /Users/samheilbron/go/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:691
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0x75622d6f672f7365?)
        /Users/samheilbron/go/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:679 +0x45
created by golang.org/x/tools/go/analysis/internal/checker.execAll in goroutine 1
        /Users/samheilbron/go/pkg/mod/golang.org/x/[email protected]/go/analysis/internal/checker/checker.go:685 +0x15a

Solution

I've taken the following approach to get around this:

1. Check out the linter code locally
2. Fix the panic
3. Rebuild the executable (there is a nice make target in the library)
4. Use that locally build executable to run ginkgolinter -fix ./...
5. I have created [BUG] Panic during checkMatchErrorAssertion with -fix nunnatsa/ginkgolinter#124 to track this bug
6. I have also opened [BUG] Custom GomegaMatcher Incorrectly Compares to MatchError nunnatsa/ginkgolinter#125 to track another behavior I came across

Solution Update

Both issues that I had hit with the Ginkgo linter (nunnatsa/ginkgolinter#124 (comment)) have been fixed in a recent release.

Interesting decisions

• Currently I solve a number of the lint issues that are in tests, although I don't require tests to pass the linter in CI
• The lint job is NOT YET a required job in CI. I intend to make it required once this merges, since it should not fail on PRs.

Testing steps

• I ran make analyze with tests enabled, and fixed a number of lint issues in tests
• I ran make analyze with tests disabled, and confirmed that no errors existed
• Since I am changing test code (even though it should not be a functional change), I opted to let build-bot and github actions run

Notes for reviewers

Next Steps

I intend to follow up this PR with more fixes for tests, and see if we can enable test linting in CI

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[solo-changelog-bot]

Issues linked to changelog:
solo-io#6686

[jbohanon]

This is AWESOME!

Two other possible rules:

• usage of deprecated io/ioutil
• Async assertions without defined timeout and interval

[jbohanon]

🚀 this is excellent

[jbohanon]

🚀

[ben-taussig-solo]

Another potential rule to consider -- instantiation of Ginkgo control blocks (Describe, Context, It, etc.) within loops

We try to do this in the happypath tests which leads to misleading results https://github.com/solo-io/gloo/blob/dd567e83fc8137ae6a0ae5312b9a765348ef45ee/test/e2e/happypath_test.go#L88-L90

[inFocus7]

👍🏼 🚀

[sam-heilbron]

trigger bulldozer