From 483b06e5d50d8f9e5abb1d7089f9ffcee089adb1 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sun, 7 Aug 2022 12:11:41 -0700 Subject: [PATCH 1/4] Update Go version --- go.mod | 4 ++-- go.sum | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 6ab11bf..63d1183 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/oschwald/geoip2-golang -go 1.17 +go 1.18 require ( github.com/oschwald/maxminddb-golang v1.9.0 @@ -10,6 +10,6 @@ require ( require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - golang.org/x/sys v0.0.0-20220325203850-36772127a21f // indirect + golang.org/x/sys v0.0.0-20220804214406-8e32c043e418 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 56808da..ba4dab9 100644 --- a/go.sum +++ b/go.sum @@ -10,8 +10,8 @@ github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSS github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.4 h1:wZRexSlwd7ZXfKINDLsO4r7WBt3gTKONc6K/VesHvHM= github.com/stretchr/testify v1.7.4/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= -golang.org/x/sys v0.0.0-20220325203850-36772127a21f h1:TrmogKRsSOxRMJbLYGrB4SBbW+LJcEllYBLME5Zk5pU= -golang.org/x/sys v0.0.0-20220325203850-36772127a21f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220804214406-8e32c043e418 h1:9vYwv7OjYaky/tlAeD7C4oC9EsPTlaFl1H2jS++V+ME= +golang.org/x/sys v0.0.0-20220804214406-8e32c043e418/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From ab86d30fa7a001e932981dab47fc57fb983048d5 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sun, 7 Aug 2022 12:13:53 -0700 Subject: [PATCH 2/4] Update golangci-lint config and fix minor issues --- .golangci.toml | 435 ++++++++++++++++++++++++++++++++++++++++++++++++- reader.go | 10 +- reader_test.go | 6 +- 3 files changed, 435 insertions(+), 16 deletions(-) diff --git a/.golangci.toml b/.golangci.toml index c227c6c..b4f7e6a 100644 --- a/.golangci.toml +++ b/.golangci.toml @@ -1,55 +1,472 @@ [run] deadline = "10m" + tests = true [linters] disable-all = true enable = [ + "asciicheck", + "bidichk", "bodyclose", + "containedctx", + "contextcheck", "deadcode", "depguard", + "durationcheck", "errcheck", + "errchkjson", + "errname", + "errorlint", "exportloopref", + "forbidigo", + #"forcetypeassert", "goconst", "gocyclo", "gocritic", + "godot", "gofumpt", + "gomodguard", "gosec", "gosimple", "govet", + "grouper", "ineffassign", + "lll", + "makezero", + "maintidx", "misspell", "nakedret", + "nilerr", "noctx", "nolintlint", + "nosprintfhostport", + "predeclared", "revive", + "rowserrcheck", "sqlclosecheck", "staticcheck", "structcheck", "stylecheck", + "tenv", + "tparallel", "typecheck", "unconvert", "unparam", "unused", "varcheck", "vetshadow", + "wastedassign", + ] + +# Please note that we only use depguard for stdlib as gomodguard only +# supports modules currently. See https://github.com/ryancurrah/gomodguard/issues/12 +[linters-settings.depguard] + list-type = "blacklist" + include-go-root = true + packages = [ + # ioutil is deprecated. The functions have been moved elsewhere: + # https://golang.org/doc/go1.16#ioutil + "io/ioutil", ] [linters-settings.errcheck] + # Don't allow setting of error to the blank identifier. If there is a legtimate + # reason, there should be a nolint with an explanation. + check-blank = true + + exclude-functions = [ + # If we are rolling back a transaction, we are often already in an error + # state. + '(*database/sql.Tx).Rollback', + + # It is reasonable to ignore errors if Cleanup fails in most cases. + '(*github.com/google/renameio/v2.PendingFile).Cleanup', + + # We often don't care if removing a file failed (e.g., it doesn't exist) + 'os.Remove', + 'os.RemoveAll', + ] + + # Ignoring Close so that we don't have to have a bunch of + # `defer func() { _ = r.Close() }()` constructs when we + # don't actually care about the error. ignore = "Close,fmt:.*" +[linters-settings.errorlint] + errorf = true + asserts = true + comparison = true + +[linters-settings.exhaustive] + default-signifies-exhaustive = true + +[linters-settings.forbidigo] + # Forbid the following identifiers + forbid = [ + "^minFraud*", + "^maxMind*", + ] + +[linters-settings.gocritic] + enabled-checks = [ + "appendAssign", + "appendCombine", + "argOrder", + "assignOp", + "badCall", + "badCond", + "badLock", + "badRegexp", + "badSorting", + "boolExprSimplify", + "builtinShadow", + "builtinShadowDecl", + "captLocal", + "caseOrder", + "codegenComment", + "commentedOutCode", + "commentedOutImport", + "commentFormatting", + "defaultCaseOrder", + # Revive's defer rule already captures this. This caught no extra cases. + # "deferInLoop", + "deferUnlambda", + "deprecatedComment", + "docStub", + "dupArg", + "dupBranchBody", + "dupCase", + "dupImport", + "dupSubExpr", + "dynamicFmtString", + "elseif", + "emptyDecl", + "emptyFallthrough", + "emptyStringTest", + "equalFold", + "evalOrder", + "exitAfterDefer", + "exposedSyncMutex", + "externalErrorReassign", + # Given that all of our code runs on Linux and the / separate should + # work fine, this seems less important. + # "filepathJoin", + "flagDeref", + "flagName", + "hexLiteral", + "ifElseChain", + "importShadow", + "indexAlloc", + "initClause", + "ioutilDeprecated", + "mapKey", + "methodExprCall", + "nestingReduce", + "newDeref", + "nilValReturn", + "octalLiteral", + "offBy1", + "paramTypeCombine", + "preferDecodeRune", + "preferFilepathJoin", + "preferFprint", + "preferStringWriter", + "preferWriteByte", + "ptrToRefParam", + "rangeExprCopy", + "rangeValCopy", + "redundantSprint", + "regexpMust", + "regexpPattern", + # This might be good, but I don't think we want to encourage + # significant changes to regexes as we port stuff from Perl. + # "regexpSimplify", + "ruleguard", + "singleCaseSwitch", + "sliceClear", + "sloppyLen", + # This seems like it might also be good, but a lot of existing code + # fails. + # "sloppyReassign", + "returnAfterHttpError", + "sloppyTypeAssert", + "sortSlice", + "sprintfQuotedString", + "sqlQuery", + "stringsCompare", + "stringXbytes", + "switchTrue", + "syncMapLoadAndDelete", + "timeExprSimplify", + "todoCommentWithoutDetail", + "tooManyResultsChecker", + "truncateCmp", + "typeAssertChain", + "typeDefFirst", + "typeSwitchVar", + "typeUnparen", + "underef", + "unlabelStmt", + "unlambda", + # I am not sure we would want this linter and a lot of existing + # code fails. + # "unnamedResult", + "unnecessaryBlock", + "unnecessaryDefer", + "unslice", + "valSwap", + "weakCond", + "wrapperFunc", + "yodaStyleExpr", + # This requires explanations for "nolint" directives. This would be + # nice for gosec ones, but I am not sure we want it generally unless + # we can get the false positive rate lower. + # "whyNoLint" + ] + [linters-settings.gofumpt] extra-rules = true + lang-version = "1.18" + +[linters-settings.govet] + "enable-all" = true + +[linters-settings.lll] + line-length = 120 + tab-width = 4 + +[linters-settings.nolintlint] + allow-leading-space = false + allow-unused = false + allow-no-explanation = ["lll", "misspell"] + require-explanation = true + require-specific = true + +[linters-settings.revive] + ignore-generated-header = true + severity = "warning" + + # This might be nice but it is so common that it is hard + # to enable. + # [[linters-settings.revive.rules]] + # name = "add-constant" + + # [[linters-settings.revive.rules]] + # name = "argument-limit" + + [[linters-settings.revive.rules]] + name = "atomic" + + [[linters-settings.revive.rules]] + name = "bare-return" + + [[linters-settings.revive.rules]] + name = "blank-imports" + + [[linters-settings.revive.rules]] + name = "bool-literal-in-expr" + + [[linters-settings.revive.rules]] + name = "call-to-gc" + + # [[linters-settings.revive.rules]] + # name = "cognitive-complexity" + + # Probably a good rule, but we have a lot of names that + # only have case differences. + # [[linters-settings.revive.rules]] + # name = "confusing-naming" + + # [[linters-settings.revive.rules]] + # name = "confusing-results" + + [[linters-settings.revive.rules]] + name = "constant-logical-expr" + + [[linters-settings.revive.rules]] + name = "context-as-argument" + + [[linters-settings.revive.rules]] + name = "context-keys-type" + + # [[linters-settings.revive.rules]] + # name = "cyclomatic" + + # [[linters-settings.revive.rules]] + # name = "deep-exit" + + [[linters-settings.revive.rules]] + name = "defer" + + [[linters-settings.revive.rules]] + name = "dot-imports" + + [[linters-settings.revive.rules]] + name = "duplicated-imports" + + [[linters-settings.revive.rules]] + name = "early-return" + + [[linters-settings.revive.rules]] + name = "empty-block" + + [[linters-settings.revive.rules]] + name = "empty-lines" + + [[linters-settings.revive.rules]] + name = "errorf" + + [[linters-settings.revive.rules]] + name = "error-naming" + + [[linters-settings.revive.rules]] + name = "error-return" + + [[linters-settings.revive.rules]] + name = "error-strings" + + [[linters-settings.revive.rules]] + name = "exported" -[issues] -exclude-use-default = false + # [[linters-settings.revive.rules]] + # name = "file-header" - [[issues.exclude-rules]] + # We have a lot of flag parameters. This linter probably makes + # a good point, but we would need some cleanup or a lot of nolints. + # [[linters-settings.revive.rules]] + # name = "flag-parameter" + + # [[linters-settings.revive.rules]] + # name = "function-result-limit" + + [[linters-settings.revive.rules]] + name = "get-return" + + [[linters-settings.revive.rules]] + name = "identical-branches" + + [[linters-settings.revive.rules]] + name = "if-return" + + [[linters-settings.revive.rules]] + name = "imports-blacklist" + + [[linters-settings.revive.rules]] + name = "import-shadowing" + + [[linters-settings.revive.rules]] + name = "increment-decrement" + + [[linters-settings.revive.rules]] + name = "indent-error-flow" + + # [[linters-settings.revive.rules]] + # name = "line-length-limit" + + # [[linters-settings.revive.rules]] + # name = "max-public-structs" + + [[linters-settings.revive.rules]] + name = "modifies-parameter" + + [[linters-settings.revive.rules]] + name = "modifies-value-receiver" + + # We frequently use nested structs, particularly in tests. + # [[linters-settings.revive.rules]] + # name = "nested-structs" + + [[linters-settings.revive.rules]] + name = "optimize-operands-order" + + [[linters-settings.revive.rules]] + name = "package-comments" + + [[linters-settings.revive.rules]] + name = "range" + + [[linters-settings.revive.rules]] + name = "range-val-address" + + [[linters-settings.revive.rules]] + name = "range-val-in-closure" + + [[linters-settings.revive.rules]] + name = "receiver-naming" + + [[linters-settings.revive.rules]] + name = "redefines-builtin-id" + + [[linters-settings.revive.rules]] + name = "string-of-int" + + [[linters-settings.revive.rules]] + name = "struct-tag" + + [[linters-settings.revive.rules]] + name = "superfluous-else" + + [[linters-settings.revive.rules]] + name = "time-naming" + + [[linters-settings.revive.rules]] + name = "unconditional-recursion" + + [[linters-settings.revive.rules]] + name = "unexported-naming" + + [[linters-settings.revive.rules]] + name = "unexported-return" + + # This is covered elsewhere and we want to ignore some + # functions such as fmt.Fprintf. + # [[linters-settings.revive.rules]] + # name = "unhandled-error" + + [[linters-settings.revive.rules]] + name = "unnecessary-stmt" + + [[linters-settings.revive.rules]] + name = "unreachable-code" + + [[linters-settings.revive.rules]] + name = "unused-parameter" + + # We generally have unused receivers in tests for meeting the + # requirements of an interface. + # [[linters-settings.revive.rules]] + # name = "unused-receiver" + + # This probably makes sense after we upgrade to 1.18 + # [[linters-settings.revive.rules]] + # name = "use-any" + + [[linters-settings.revive.rules]] + name = "useless-break" + + [[linters-settings.revive.rules]] + name = "var-declaration" + + [[linters-settings.revive.rules]] + name = "var-naming" + + [[linters-settings.revive.rules]] + name = "waitgroup-by-value" + +[linters-settings.unparam] + check-exported = true + +[[issues.exclude-rules]] linters = [ - "gosec" + "govet" ] - - # G304 - Potential file inclusion via variable (gosec) - # G404 - "Use of weak random number generator (math/rand instead of crypto/rand)" - # We only use this in tests. - text = "G304|G404" + # we want to enable almost all govet rules. It is easier to just filter out + # the ones we don't want: + # + # * fieldalignment - way too noisy. Although it is very useful in particular + # cases where we are trying to use as little memory as possible, having + # it go off on every struct isn't helpful. + # * shadow - although often useful, it complains about _many_ err + # shadowing assignments and some others where shadowing is clear. + text = "^(fieldalignment|shadow)" diff --git a/reader.go b/reader.go index 6bc120e..f8439ec 100644 --- a/reader.go +++ b/reader.go @@ -245,7 +245,7 @@ type UnknownDatabaseTypeError struct { } func (e UnknownDatabaseTypeError) Error() string { - return fmt.Sprintf(`geoip2: reader does not support the "%s" database type`, + return fmt.Sprintf(`geoip2: reader does not support the %q database type`, e.DatabaseType) } @@ -362,7 +362,7 @@ func (r *Reader) AnonymousIP(ipAddress net.IP) (*AnonymousIP, error) { } // ASN takes an IP address as a net.IP struct and returns a ASN struct and/or -// an error +// an error. func (r *Reader) ASN(ipAddress net.IP) (*ASN, error) { if isASN&r.databaseType == 0 { return nil, InvalidMethodError{"ASN", r.Metadata().DatabaseType} @@ -373,7 +373,7 @@ func (r *Reader) ASN(ipAddress net.IP) (*ASN, error) { } // ConnectionType takes an IP address as a net.IP struct and returns a -// ConnectionType struct and/or an error +// ConnectionType struct and/or an error. func (r *Reader) ConnectionType(ipAddress net.IP) (*ConnectionType, error) { if isConnectionType&r.databaseType == 0 { return nil, InvalidMethodError{"ConnectionType", r.Metadata().DatabaseType} @@ -384,7 +384,7 @@ func (r *Reader) ConnectionType(ipAddress net.IP) (*ConnectionType, error) { } // Domain takes an IP address as a net.IP struct and returns a -// Domain struct and/or an error +// Domain struct and/or an error. func (r *Reader) Domain(ipAddress net.IP) (*Domain, error) { if isDomain&r.databaseType == 0 { return nil, InvalidMethodError{"Domain", r.Metadata().DatabaseType} @@ -395,7 +395,7 @@ func (r *Reader) Domain(ipAddress net.IP) (*Domain, error) { } // ISP takes an IP address as a net.IP struct and returns a ISP struct and/or -// an error +// an error. func (r *Reader) ISP(ipAddress net.IP) (*ISP, error) { if isISP&r.databaseType == 0 { return nil, InvalidMethodError{"ISP", r.Metadata().DatabaseType} diff --git a/reader_test.go b/reader_test.go index 70cbd17..b46c76f 100644 --- a/reader_test.go +++ b/reader_test.go @@ -240,7 +240,7 @@ func TestISP(t *testing.T) { assert.Equal(t, "Verizon Wireless", record.Organization) } -// This ensures the compiler does not optimize away the function call +// This ensures the compiler does not optimize away the function call. var cityResult *City func BenchmarkCity(b *testing.B) { @@ -250,6 +250,7 @@ func BenchmarkCity(b *testing.B) { } defer db.Close() + //nolint:gosec // this is just a benchmark r := rand.New(rand.NewSource(0)) var city *City @@ -265,7 +266,7 @@ func BenchmarkCity(b *testing.B) { cityResult = city } -// This ensures the compiler does not optimize away the function call +// This ensures the compiler does not optimize away the function call. var asnResult *ASN func BenchmarkASN(b *testing.B) { @@ -275,6 +276,7 @@ func BenchmarkASN(b *testing.B) { } defer db.Close() + //nolint:gosec // this is just a benchmark r := rand.New(rand.NewSource(0)) var asn *ASN From a66d003242edd081d0d2710023a1fe007ad68aab Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sun, 7 Aug 2022 12:18:10 -0700 Subject: [PATCH 3/4] Update deps --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 63d1183..d97bbe3 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module github.com/oschwald/geoip2-golang go 1.18 require ( - github.com/oschwald/maxminddb-golang v1.9.0 - github.com/stretchr/testify v1.7.4 + github.com/oschwald/maxminddb-golang v1.10.0 + github.com/stretchr/testify v1.8.0 ) require ( diff --git a/go.sum b/go.sum index ba4dab9..9044f99 100644 --- a/go.sum +++ b/go.sum @@ -1,15 +1,15 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/oschwald/maxminddb-golang v1.9.0 h1:tIk4nv6VT9OiPyrnDAfJS1s1xKDQMZOsGojab6EjC1Y= -github.com/oschwald/maxminddb-golang v1.9.0/go.mod h1:TK+s/Z2oZq0rSl4PSeAEoP0bgm82Cp5HyvYbt8K3zLY= +github.com/oschwald/maxminddb-golang v1.10.0 h1:Xp1u0ZhqkSuopaKmk1WwHtjF0H9Hd9181uj2MQ5Vndg= +github.com/oschwald/maxminddb-golang v1.10.0/go.mod h1:Y2ELenReaLAZ0b400URyGwvYxHV1dLIxBuyOsyYjHK0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.7.4 h1:wZRexSlwd7ZXfKINDLsO4r7WBt3gTKONc6K/VesHvHM= -github.com/stretchr/testify v1.7.4/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= golang.org/x/sys v0.0.0-20220804214406-8e32c043e418 h1:9vYwv7OjYaky/tlAeD7C4oC9EsPTlaFl1H2jS++V+ME= golang.org/x/sys v0.0.0-20220804214406-8e32c043e418/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= From 872132ce9ac45eaee1480c963c731aac25e365a3 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sun, 7 Aug 2022 12:19:02 -0700 Subject: [PATCH 4/4] Test on 1.18 and 1.19 --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 6bf650a..618c5ee 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -8,7 +8,7 @@ jobs: name: Build strategy: matrix: - go-version: [1.17.x, 1.18.x] + go-version: [1.18.x, 1.19.x] platform: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.platform }} steps: