Integrating golangci-lint #1179

wilsonehusin · 2020-10-20T17:55:59Z

What this PR does / why we need it:
Repository housecleaning.

Which issue(s) this PR fixes
Partially addresses #1176, baby steps towards #1178

Special notes for your reviewer:
Seems like GitHub is not running the actions on this PR as vmware-tanzu, but it's available on my fork

Release note:

NONE

pkg/client/logs.go

vladimirvivien

This is a great first start. Let's discuss further.

cmd/sonobuoy/app/args.go

cmd/sonobuoy/app/common.go

vladimirvivien · 2020-10-21T11:57:36Z

cmd/sonobuoy/app/gen.go

@@ -279,7 +278,7 @@ func (g *genFlags) resolveConfig() *config.Config {
 		// Set these values as if the user had requested the defaults.
 		if g.genflags != nil {
 			for _, v := range conf.PluginSelections {
-				g.genflags.Lookup("plugin").Value.Set(v.Name)
+				g.genflags.Lookup("plugin").Value.Set(v.Name) //nolint:errcheck


Is //nolint:errcheck skipping this as an error check ? I see several of these. It may be worth it to actually fix these in another pass instead of having that comment all over the place.

//nolint:errcheck will tell golangci-lint to not mark this as errcheck lint failure. the only resolutions I can think of are:

// practically silencing error _ = g.genflags.Lookup("plugin").Value.Set(v.Name)

or

// potentially changing behavior, because previously we ignored this error if they exist err := g.genflags.Lookup("plugin").Value.Set(v.Name) if err != nil { ... }

is there other alternatives?

I think there may be one more option. Looking at the doc for errcheck linter, it supports an external file where you can put package, functions, methods to skip.

Take a look here https://github.com/kisielk/errcheck#excluding-functions

cmd/sonobuoy/app/results.go

vladimirvivien · 2020-10-21T11:59:53Z

cmd/sonobuoy/app/version.go

-	fmt.Println(fmt.Sprintf("MinimumKubeVersion: %s", buildinfo.MinimumKubeVersion))
-	fmt.Println(fmt.Sprintf("MaximumKubeVersion: %s", buildinfo.MaximumKubeVersion))
-	fmt.Println(fmt.Sprintf("GitSHA: %s", buildinfo.GitSHA))
+	fmt.Printf("Sonobuoy Version: %s\n", buildinfo.Version)


Good catch here.

pkg/client/preflight.go

vladimirvivien · 2020-10-21T12:02:57Z

pkg/client/run.go

@@ -121,6 +121,12 @@ func (c *SonobuoyClient) RunManifest(cfg *RunConfig, manifest []byte) error {
 				seenStatus = true
 			}

+			// if nil below was added for coverage on staticcheck
+			// TODO: ensure this is the desired behavior
+			if status == nil {


Let's discuss this further. Agreed we need to understand this is not adding new side effects.

so after taking another look, it seems that in scenario where status is nil, previous behavior would have runtime de-referencing error on switch{}. so prooooobably ok to have a more channeled error delivery?

vladimirvivien · 2020-10-21T12:04:14Z

pkg/config/loader.go

@@ -55,7 +55,7 @@ func LoadConfig() (*Config, error) {
 	}
 	defer jsonFile.Close()

-	b, err := ioutil.ReadAll(jsonFile)
+	b, _ := ioutil.ReadAll(jsonFile)


Error handling: the error should not be silenced here.

Same thing here, let's add the error handling logic needed. Probably should add something like this

b, err := ioutil.ReadAll(jsonFile) if err != nil { return nil, errors.Wrap(err, "read config") }

vladimirvivien · 2020-10-21T12:05:46Z

pkg/discovery/pods.go

 	// Only set values if they have values greater than 0 (as in they user specified).
-	limitBytes := podLogLimits.SizeLimitBytes(0)
-	sinceSeconds := int64(podLogLimits.TimeLimitDuration(0) / time.Second)
+	limitBytes := podLogLimits.SizeLimitBytes(0)                           //nolint:staticcheck


This is good for now. Lets investigate what would satisfy this check and if it requires package upgrade then we can handle that in a different PR.

wilsonehusin

some clarifications on changes and responses. I've been defaulting to silencing previously unhandled errors to preserve behavior, but I agree with the general sentiment that we should ensure they are handled appropriately -- whether to ignore (mark as insignificant) or surface it to user.

cmd/sonobuoy/app/args.go

cmd/sonobuoy/app/common.go

wilsonehusin · 2020-10-21T16:07:00Z

cmd/sonobuoy/app/gen.go

@@ -279,7 +278,7 @@ func (g *genFlags) resolveConfig() *config.Config {
 		// Set these values as if the user had requested the defaults.
 		if g.genflags != nil {
 			for _, v := range conf.PluginSelections {
-				g.genflags.Lookup("plugin").Value.Set(v.Name)
+				g.genflags.Lookup("plugin").Value.Set(v.Name) //nolint:errcheck


//nolint:errcheck will tell golangci-lint to not mark this as errcheck lint failure. the only resolutions I can think of are:

// practically silencing error _ = g.genflags.Lookup("plugin").Value.Set(v.Name)

or

// potentially changing behavior, because previously we ignored this error if they exist err := g.genflags.Lookup("plugin").Value.Set(v.Name) if err != nil { ... }

is there other alternatives?

cmd/sonobuoy/app/results.go

pkg/client/preflight.go

vladimirvivien

Looks good. Left a link for you to check out for the errcheck linter

vladimirvivien · 2020-10-26T19:01:29Z

cmd/sonobuoy/app/gen.go

@@ -279,7 +278,7 @@ func (g *genFlags) resolveConfig() *config.Config {
 		// Set these values as if the user had requested the defaults.
 		if g.genflags != nil {
 			for _, v := range conf.PluginSelections {
-				g.genflags.Lookup("plugin").Value.Set(v.Name)
+				g.genflags.Lookup("plugin").Value.Set(v.Name) //nolint:errcheck


I think there may be one more option. Looking at the doc for errcheck linter, it supports an external file where you can put package, functions, methods to skip.

Take a look here https://github.com/kisielk/errcheck#excluding-functions

vladimirvivien

Ok, added some comments about error handling in several places.

cmd/sonobuoy/app/results.go

vladimirvivien · 2020-10-26T20:56:22Z

cmd/sonobuoy/app/worker.go

-		err := runGather(false)
+		// staticcheck somehow flags err as unused when it's actually returned
+		// at the end of this function
+		err := runGather(false) //nolint:staticcheck


For this one, the static analysis is correct, one of the switch case sleeps forever. So, add a err handling block right after the rungGather call

if err := runGather(flase); err != nil { return err } ... // return nil at the end of func return nil

vladimirvivien · 2020-10-26T20:57:24Z

pkg/client/interfaces.go

@@ -30,6 +30,8 @@ import (
 )

 // ConfigValidator allows the command configurations to be validated.
+// TODO: investigate for removal, currently only used in interfaces_test.go


If this is only used in a test, I would move this in the test file where it is used.

vladimirvivien · 2020-10-26T21:01:56Z

pkg/config/loader.go

@@ -55,7 +55,7 @@ func LoadConfig() (*Config, error) {
 	}
 	defer jsonFile.Close()

-	b, err := ioutil.ReadAll(jsonFile)
+	b, _ := ioutil.ReadAll(jsonFile)


Same thing here, let's add the error handling logic needed. Probably should add something like this

b, err := ioutil.ReadAll(jsonFile) if err != nil { return nil, errors.Wrap(err, "read config") }

vladimirvivien · 2020-10-26T21:07:20Z

pkg/discovery/discovery.go

@@ -152,7 +152,7 @@ func Run(restConf *rest.Config, cfg *config.Config) (errCount int) {

 	// 5. Run the queries
 	recorder := NewQueryRecorder()
-	clusterResources, nsResources, err := getAllFilteredResources(apiHelper, cfg.Resources)
+	clusterResources, nsResources, _ := getAllFilteredResources(apiHelper, cfg.Resources) // TODO: handle error checking


Same deal here, let's add an error handler

clusterResources, nsResources, err := getAllFilteredResources(apiHelper, cfg.Resources) if err != nil { errlog.LogError(errors.Wrap(err, "getting all filtered resources")) return errCount + 1 }

vladimirvivien

LGTM

vladimirvivien · 2020-10-27T16:05:07Z

@wilsonehusin Please squash commits and include signoff

- source code lint with golangci-lint - ignore low-priority errchecks (for now) - add GitHub actions for linting CI Signed-off-by: Wilson E. Husin <[email protected]>

wilsonehusin commented Oct 20, 2020

View reviewed changes

pkg/client/logs.go Show resolved Hide resolved

wilsonehusin marked this pull request as ready for review October 20, 2020 23:04

wilsonehusin requested a review from vladimirvivien October 20, 2020 23:35

vladimirvivien reviewed Oct 21, 2020

View reviewed changes

wilsonehusin commented Oct 21, 2020

View reviewed changes

wilsonehusin requested a review from vladimirvivien October 26, 2020 18:33

vladimirvivien reviewed Oct 26, 2020

View reviewed changes

wilsonehusin requested a review from vladimirvivien October 26, 2020 20:36

vladimirvivien reviewed Oct 26, 2020

View reviewed changes

wilsonehusin requested a review from vladimirvivien October 26, 2020 21:42

wilsonehusin mentioned this pull request Oct 27, 2020

Inconsistent unit test result #1181

Closed

vladimirvivien approved these changes Oct 27, 2020

View reviewed changes

Add golangci-lint on GitHub Actions with fixes

10dfea0

- source code lint with golangci-lint - ignore low-priority errchecks (for now) - add GitHub actions for linting CI Signed-off-by: Wilson E. Husin <[email protected]>

wilsonehusin merged commit 7419e1d into vmware-tanzu:master Oct 27, 2020

wilsonehusin deleted the golangci-lint branch October 28, 2020 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating golangci-lint #1179

Integrating golangci-lint #1179

wilsonehusin commented Oct 20, 2020 •

edited

Loading

vladimirvivien left a comment

vladimirvivien Oct 21, 2020

wilsonehusin Oct 21, 2020

vladimirvivien Oct 26, 2020

vladimirvivien Oct 21, 2020

vladimirvivien Oct 21, 2020

wilsonehusin Oct 26, 2020

vladimirvivien Oct 21, 2020

vladimirvivien Oct 26, 2020

vladimirvivien Oct 21, 2020

wilsonehusin left a comment

wilsonehusin Oct 21, 2020

vladimirvivien left a comment

vladimirvivien Oct 26, 2020

vladimirvivien left a comment

vladimirvivien Oct 26, 2020

vladimirvivien Oct 26, 2020

vladimirvivien Oct 26, 2020

vladimirvivien Oct 26, 2020

vladimirvivien left a comment

vladimirvivien commented Oct 27, 2020

Integrating golangci-lint #1179

Integrating golangci-lint #1179

Conversation

wilsonehusin commented Oct 20, 2020 • edited Loading

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonehusin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien left a comment

Choose a reason for hiding this comment

vladimirvivien commented Oct 27, 2020

wilsonehusin commented Oct 20, 2020 •

edited

Loading