[ci] Add `errcheck` to golangci-lint #7996

ajm188 · 2021-04-29T12:40:40Z

Description

This PR enables the errcheck linter as part of the golangci-lint linter suite, and then sets up a bunch of configs to allow us to progressively enable this linter on more and more parts of the codebase.

To make this work, I needed to bump the version of golangci-lint to v1.39.0, since earlier versions were not correctly working with the errcheck_excludes.txt file. I can't go all the way to the latest version (v1.40.1) because that starts to complain about stuff in the fuzz tests, and I didn't want to touch those in this change haha.

After I got the config into a working state, I then moved through and fixed up a bunch of the now-complaining errcheck failures. I stuck to packages that had relatively few failures, and to errcheck violations that I thought would be pretty uncontroversial to fix. The rest of the packages I've left as ignored in our .golangci.yml config, and as we gradually fix those instances we can remove those entries from the config.

Related Issue(s)

Closes #8222

Checklist

Tests were added or are not required -- n/a
Documentation was added or is not required

Deployment Notes

harshit-gangal · 2021-04-29T12:53:15Z

@ajm188 AFAIK, in draft mode also the CI tests run :)

ajm188 · 2021-04-29T13:32:49Z

huh! i have a vague memory of turning a PR back to draft mode to debug some tests and none of the CIs were running. happy to be wrong though!

ajm188 · 2021-05-31T19:49:08Z

go/cmd/rulesctl/cmd/list.go

@@ -40,11 +40,13 @@ func List() *cobra.Command {
 				out = rules
 			}
 		} else {
-			out = rules.Find(listOptName)
-			if listOptNamesOnly && out != nil {
+			rule := rules.Find(listOptName)


See 6ab6b47 for why this change

go/vt/binlog/binlogplayer/binlog_player.go

ajm188 · 2021-05-31T19:51:31Z

go/vt/orchestrator/collection/collection.go

@@ -283,7 +283,7 @@ func (c *Collection) Append(m Metric) error {
 	c.Lock()         //nolint SA5011: possible nil pointer dereference
 	defer c.Unlock() //nolint SA5011: possible nil pointer dereference
 	// we don't want to add nil metrics
-	if c == nil {
+	if c == nil { //nolint SA5011: redundant nil pointer check - maybe this was supposed to check if m == nil ?


@shlomi-noach calling your particular attention to this line; I think this should be checking if m == nil (since we checked c == nil just before locking above) but wanted to confirm with you first.

@ajm188 I believe you're right, this should be m == nil. Took me a while to understand why I have no recollection of this code, it was a contribution and I'm not too familiar with it.

shlomi-noach

LGTM

shlomi-noach · 2021-06-02T04:33:08Z

go/vt/orchestrator/collection/collection.go

@@ -283,7 +283,7 @@ func (c *Collection) Append(m Metric) error {
 	c.Lock()         //nolint SA5011: possible nil pointer dereference
 	defer c.Unlock() //nolint SA5011: possible nil pointer dereference
 	// we don't want to add nil metrics
-	if c == nil {
+	if c == nil { //nolint SA5011: redundant nil pointer check - maybe this was supposed to check if m == nil ?


@ajm188 I believe you're right, this should be m == nil. Took me a while to understand why I have no recollection of this code, it was a contribution and I'm not too familiar with it.

deepthi

LGTM.
A couple of things:

Review the nolint directives, perhaps some of them should change to ignore the return value instead
It will be nice to update the version in https://github.com/vitessio/vitess/blob/main/misc/git/hooks/golangci-lint

deepthi · 2021-06-11T23:30:14Z

go/cmd/vtctl/vtctl.go

@@ -85,7 +85,7 @@ func main() {
 	startMsg := fmt.Sprintf("USER=%v SUDO_USER=%v %v", os.Getenv("USER"), os.Getenv("SUDO_USER"), strings.Join(os.Args, " "))

 	if syslogger, err := syslog.New(syslog.LOG_INFO, "vtctl "); err == nil {
-		syslogger.Info(startMsg)
+		syslogger.Info(startMsg) // nolint:errcheck


nit: why not _ = ... instead of nolint?

Signed-off-by: Andrew Mason <[email protected]>

This is an often-surprising Go quirk, so I will explain a bit: An interface has two components: the concrete type, and the value. If both of these are `nil`, then the interface is equal to `nil`. If the concrete type is non-nil, even if the value for that type is nil, then the interface is non-nil. For a toy example, see: https://play.golang.org/p/0uLrBekVg_y. In this specific case, we were taking `rules.Find`, which returns `*rules.Rule` and assigning that to a variable with the type of `interface{}`. Since we have a concrete type for the interface, the `out != nil` check will never be false. The fix here is to use a second variable, which will have the concrete type, and check if the pointer is nil, which is what we actually wanted here. Signed-off-by: Andrew Mason <[email protected]>

Signed-off-by: Andrew Mason <[email protected]>

…lint directives Signed-off-by: Andrew Mason <[email protected]>

Signed-off-by: Andrew Mason <[email protected]>

deepthi · 2021-08-26T20:55:37Z

Still need to update the version we download in the hook (if the binary doesn't exist locally).

vitess/misc/git/hooks/golangci-lint

Line 28 in 8b7a4cb

    
           curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.34.1

Also, do we need to announce/warn people in #developers that they will need to upgrade their installed version of the linter?

Signed-off-by: Andrew Mason <[email protected]>

ajm188 · 2021-08-26T21:09:59Z

yeah, i was planning to post in #developers after i merged!

ajm188 marked this pull request as draft April 29, 2021 13:32

ajm188 mentioned this pull request Apr 29, 2021

[wrangler|topotools] Migrate UpdateShardRecords, RefreshTabletsByShard, and {Get,Save}RoutingRules #7965

Merged

2 tasks

ajm188 force-pushed the am_errcheck branch from e021ce2 to 0306492 Compare May 29, 2021 12:18

ajm188 added Component: Build/CI Type: CI/Build labels May 29, 2021

ajm188 force-pushed the am_errcheck branch 6 times, most recently from eefb8a6 to 8dca6f7 Compare May 30, 2021 01:49

ajm188 changed the title ~~[WIP | Do not merge] add errcheck to golangci-lint~~ [ci] Add errcheck to golangci-lint May 30, 2021

ajm188 requested review from falun, systay and deepthi May 31, 2021 19:45

ajm188 marked this pull request as ready for review May 31, 2021 19:45

ajm188 requested review from doeg, GuptaManan100, harshit-gangal, rafael, rohit-nayak-ps, shlomi-noach and vmg as code owners May 31, 2021 19:45

ajm188 commented May 31, 2021

View reviewed changes

go/vt/binlog/binlogplayer/binlog_player.go Show resolved Hide resolved

ajm188 commented May 31, 2021

View reviewed changes

shlomi-noach reviewed Jun 2, 2021

View reviewed changes

ajm188 force-pushed the am_errcheck branch from 4d7fbcb to 9733842 Compare June 4, 2021 02:28

ajm188 force-pushed the am_errcheck branch from 9733842 to 21ae0b4 Compare June 4, 2021 02:29

deepthi approved these changes Jun 11, 2021

View reviewed changes

ajm188 added 8 commits August 26, 2021 15:19

Add errcheck to golangci-lint

b803b28

Signed-off-by: Andrew Mason <[email protected]>

Fix errchecks in a few places

fd8edfd

Signed-off-by: Andrew Mason <[email protected]>

Bump golangci-lint version

65c723e

Signed-off-by: Andrew Mason <[email protected]>

fix a bunch of linter errors that are now firing

703e8f1

Signed-off-by: Andrew Mason <[email protected]>

Fix more errcheck lints

e4fef25

Signed-off-by: Andrew Mason <[email protected]>

[orchestrator] correctly check against m == nil, remove unneeded no…

6f66a22

…lint directives Signed-off-by: Andrew Mason <[email protected]>

[vttestserver] Fix errcheck violations in new tests

1218823

Signed-off-by: Andrew Mason <[email protected]>

ajm188 force-pushed the am_errcheck branch from 21ae0b4 to 1218823 Compare August 26, 2021 19:41

Update for new errcheck violations after rebase

81a2396

Signed-off-by: Andrew Mason <[email protected]>

Update hook golangci-lint version to match CI

a637713

Signed-off-by: Andrew Mason <[email protected]>

ajm188 merged commit de7f133 into vitessio:main Aug 26, 2021

ajm188 deleted the am_errcheck branch August 26, 2021 22:38

frouioui added the release notes label Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ci] Add `errcheck` to golangci-lint #7996

[ci] Add `errcheck` to golangci-lint #7996

ajm188 commented Apr 29, 2021 •

edited

Loading

harshit-gangal commented Apr 29, 2021

ajm188 commented Apr 29, 2021

ajm188 May 31, 2021

ajm188 May 31, 2021

shlomi-noach Jun 2, 2021

shlomi-noach left a comment

shlomi-noach Jun 2, 2021

deepthi left a comment

deepthi Jun 11, 2021

deepthi commented Aug 26, 2021

ajm188 commented Aug 26, 2021

[ci] Add errcheck to golangci-lint #7996

[ci] Add errcheck to golangci-lint #7996

Conversation

ajm188 commented Apr 29, 2021 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

harshit-gangal commented Apr 29, 2021

ajm188 commented Apr 29, 2021

ajm188 May 31, 2021

Choose a reason for hiding this comment

ajm188 May 31, 2021

Choose a reason for hiding this comment

shlomi-noach Jun 2, 2021

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

shlomi-noach Jun 2, 2021

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi Jun 11, 2021

Choose a reason for hiding this comment

deepthi commented Aug 26, 2021

ajm188 commented Aug 26, 2021

[ci] Add `errcheck` to golangci-lint #7996

[ci] Add `errcheck` to golangci-lint #7996

ajm188 commented Apr 29, 2021 •

edited

Loading