Upgrade golangci lint 1.39.0

[MarcosDY]

I applied changes to make lints pass, but there are some cases that we must highlight

• scopelint lint is deprecated we may replace it with exportloopref
• We are supporting old TLS for mysql because there are some cases where it is required. But we really want it? must we disable it as default and add a configuration to allow tls versiones bellow 1.2?
• we are using old k8s libraries, but upgrading it will take some time and it is not required yet, so I added a nolint for undeprecated client/fake.

I didn't add new lints to existing list, but I did a summary with list of lints that looks useful

Complete list: https://golangci-lint.run/usage/linters/

I divide it in 2 lists:

• Interesting: list of lints I think we MUST add
• Maybe: list of lints that are useful, but I'm not sure we really want, some of them are in my must list but they will require several changes that I'm not sure we really want.

Interesting

• durationcheck: check for two durations multiplied together (we have a case in a test)
• errorlint: go-errorlint is a source code linter for Go software that can be used to find code that will cause problems with the error wrapping scheme introduced in Go 1.13.
• forcetypeassert: finds forced type assertions
• goheader: Checks is file header matches to pattern
• ifshort: Checks that your code uses short syntax for if-statements whenever possible (I personally loves using ifs short version)
• nolintlint: Reports ill-formed or insufficient nolint directives (we have some nolints that are not longer required)
• rowserrcheck: checks whether Err of rows is checked successfully (in some cases someone can miss calling s.Err and verify it before accesing data)
• exportloopref: checks for pointers to enclosing loop variables (replacement for deprectated scopelint)
• wastedassign: wastedassign finds wasted assignment statements.

Maybe

• goerr113: Golang linter to check the errors handling expressions (we use %v everywhere, it will force us to use %w)
• wrapcheck: Checks that errors returned from external packages are wrapped (I think we may add it)
• dupl: search for duplicated code (It can be useful, and our code/tests has several duplications, in some cases like unit tests it is expected because we are creating expected/initial structs)
• exhaustive: check exhaustiveness of enum switch statements (It can be usefull in some cases, and force us to always put a default on switch)
• forbidigo: Forbids identifiers (serch for uses of fmt.Print, fmt.Errors, etc. It can be useful in case someone push an fmt.Print used for debuging)
• gochecknoinits: Checks that no init functions are present in Go code (init functions are executed when importing somethings, it is not a good practice to add it)
• goprintffuncname: Checks that printf-like functions are named with f at the end
• importas: Enforces consistent import aliases (it is useful and may require some configurations)
• makezero: Finds slice declarations with non-zero initial length
• nilerr: Finds the code that returns nil even if it checks that the error is not nil. (we have some cases where we return nil when error, but in those cases it is expected)
• noctx: noctx finds sending http request without context.Context (we have some cases, that we call http.Get without ctx, so we can not cancel those calls)
• prealloc: Finds slice declarations that could potentially be preallocated (code is full of this cases)
• predeclared: find code that shadows one of Go's predeclared identifiers
• revive: a replacement for golint, it support several configuration. https://github.com/mgechev/revive
• sqlclosecheck: Checks that sql.Rows and sql.Stmt are closed.
• testpackage: linter that makes you use a separate _test package (it is something that we must do but all code does not set it and will take some time to update all tets to use only public functions)
• thelper: thelper detects golang test helpers without t.Helper() call and checks the consistency of test helpers
• gomoddirectives: with some configurations allows us to prevents someone push 'replace', 'retract', and 'excludes' directives

[azdagron]

Awesome. Thanks @MarcosDY. Many of the new linters look good. Maybe we can decide which ones we want to apply and then open up a PR one at a time, introducing the new linter and the changes associated with that linter?

[azdagron]

gross. we should just get rid of the comment block.

[azdagron]

typo on MinVersion

[azdagron]

Suggested change

	tlsConf := tls.Config{} //nolint: gosec // There are scenaries where MySQL requires older tls version, and example is when compiled with yaSSL that supports only TLSv1 and TLSv1.1 protocols.
	// MySQL still allows, and in some places requires, older TLS versions. For example, when built with yaSSL, it is limited to TLSv1 and TLSv1.1.
	// TODO: consider making this more secure by default
	tlsConf := tls.Config{} //nolint: gosec // see above

[azdagron]

typo on deprecated

[azdagron]

typo on deprecated

[azdagron]

typo on deprecated

[azdagron]

I like all of the linters that you've outlined as "Interesting" with maybe the exception of goheader. I'd love to hear @evan2645's thoughts on that. We've had discussions on per-file copyrights before.

From the "maybe" column, there are a bunch there that would be a huge amount of work to support. In general I like most of them, but I'm not sure the amount of churn required to get them to lint clean would be worth it. Particularly the error wrapping related ones and the testpackage one are going to be big lifts. The latter would be especially difficult, since the tests that benefit from not doing this are usually setting hooks and stuff that we'd have to figure out a good way of injecting.

[MarcosDY]

agree testpackage will be hard to use but we can add exceptions for older tests and just be sure that new tests we add are doing a good use of testpackage.
I can remember some tests that depends of calling private functions or vars, it will be hard to migrate (in some cases we'll need refactors to be able to tests them as expected).

[azdagron]

Thanks, @MarcosDY !