Enabled golinter errcheck option (#1014)

and fixed all the resulting linter errors.

Co-authored-by: Alena Varkockova avarkockova@mesosphere.com

.golangci.yml

@@ -5,9 +5,6 @@ linters:
     - goimports
     - golint
     - misspell
-  disable:
-    # temporarily disabled until all errcheck warnings have been fixed
-    - errcheck
 run:
   skip-dirs:
     # autogenerated clientset by client-gen

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/Masterminds/sprig v2.22.0+incompatible
 	github.com/Microsoft/go-winio v0.4.14 // indirect
 	github.com/containerd/containerd v1.2.9 // indirect
+	github.com/davecgh/go-spew v1.1.1
 	github.com/docker/docker v1.4.2-0.20190916154449-92cc603036dd
 	github.com/docker/go-connections v0.4.0 // indirect
 	github.com/docker/go-units v0.4.0 // indirect
@@ -50,6 +51,7 @@ require (
 	golang.org/x/tools v0.0.0-20191025023517-2077df36852e
 	gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect
 	gopkg.in/yaml.v2 v2.2.4
+	gotest.tools v2.2.0+incompatible
 	honnef.co/go/tools v0.0.1-2019.2.3
 	k8s.io/api v0.0.0-20191016110408-35e52d86657a
 	k8s.io/apiextensions-apiserver v0.0.0-20191016113550-5357c4baaf65

pkg/controller/instance/instance_controller_suite_test.go

@@ -35,15 +35,19 @@ func TestMain(m *testing.M) {
 	t := &envtest.Environment{
 		CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crds")},
 	}
-	apis.AddToScheme(scheme.Scheme)
+
+	if err := apis.AddToScheme(scheme.Scheme); err != nil {
+		log.Fatal(err)
+	}
 
 	var err error
 	if cfg, err = t.Start(); err != nil {
 		log.Fatal(err)
 	}
 
 	code := m.Run()
-	t.Stop()
+	// t.Stop() returns an error, but since we exit in the next line anyway, it is suppressed
+	t.Stop() //nolint
 	os.Exit(code)
 }
 

pkg/kudoctl/clog/log.go

@@ -108,8 +108,8 @@ func (v Verbose) Printf(format string, args ...interface{}) {
 	}
 }
 
-// Init allows for the initialization of log via root command
-func Init(f *pflag.FlagSet, out io.Writer) {
+// InitWithFlags allows for the initialization of log via root command
+func InitWithFlags(f *pflag.FlagSet, out io.Writer) {
 	// allows for initialization of writer in testing without CLI flags
 	if f != nil {
 		f.VarP(&logging.verbosity, "v", "v", "log level for V logs")
@@ -137,6 +137,6 @@ func Errorf(format string, a ...interface{}) error {
 }
 
 func init() {
-	// expected to be overridden with Init().  This simplifies testing and default behavior
+	// expected to be overridden with InitWithFlags().  This simplifies testing and default behavior
 	logging.out = os.Stdout
 }

pkg/kudoctl/clog/log_test.go

@@ -60,12 +60,12 @@ func TestErrorf(t *testing.T) {
 	logging.out = &buf
 
 	// error f prints at level 2, no output by default
-	Errorf("error msg")
+	Errorf("error msg") //nolint
 	assert.Equal(t, "", buf.String())
 	buf.Reset()
 
 	logging.verbosity = Level(2)
 	defer func() { logging.verbosity = Level(0) }()
-	Errorf("error msg")
+	Errorf("error msg") //nolint
 	assert.Equal(t, "error msg\n", buf.String())
 }

pkg/kudoctl/cmd/get/get_test.go

@@ -7,6 +7,8 @@ import (
 	"github.com/kudobuilder/kudo/pkg/client/clientset/versioned/fake"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
+	tassert "github.com/stretchr/testify/assert"
+	"gotest.tools/assert"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -25,11 +27,7 @@ func TestValidate(t *testing.T) {
 
 	for _, tt := range tests {
 		err := validate(tt.arg)
-		if err != nil {
-			if err.Error() != tt.err {
-				t.Errorf("Expecting error message '%s' but got '%s'", tt.err, err)
-			}
-		}
+		assert.ErrorContains(t, err, tt.err)
 	}
 }
 
@@ -57,53 +55,18 @@ func TestGetInstances(t *testing.T) {
 		},
 	}
 	tests := []struct {
-		arg       []string
-		err       string
 		instances []string
 	}{
-		{nil, "expecting exactly one argument - \"instances\"", nil},                                   // 1
-		{[]string{"arg", "arg2"}, "expecting exactly one argument - \"instances\"", nil},               // 2
-		{[]string{}, "expecting exactly one argument - \"instances\"", nil},                            // 3
-		{[]string{"somethingelse"}, "expecting \"instances\" and not \"somethingelse\"", nil},          // 4
-		{[]string{"instances"}, "expecting \"instances\" and not \"somethingelse\"", []string{"test"}}, // 5
+		{[]string{"test"}},
 	}
 
-	for i, tt := range tests {
+	for _, tt := range tests {
 		kc := newTestClient()
-		kc.InstallInstanceObjToCluster(testInstance, "default")
-		instanceList, err := getInstances(kc, env.DefaultSettings)
-		if err != nil {
-			if err.Error() != tt.err {
-				t.Errorf("%d: Expecting error message '%s' but got '%s'", i+1, tt.err, err)
-			}
-		}
-		missing := compareSlice(tt.instances, instanceList)
-		for _, m := range missing {
-			t.Errorf("%d: Missed expected instance \"%v\"", i+1, m)
-		}
-	}
-}
-
-func compareSlice(real, mock []string) []string {
-	lm := len(mock)
-
-	var diff []string
-
-	for _, rv := range real {
-		i := 0
-		j := 0
-		for _, mv := range mock {
-			i++
-			if rv == mv {
-				continue
-			}
-			if rv != mv {
-				j++
-			}
-			if lm <= j {
-				diff = append(diff, rv)
-			}
+		if _, err := kc.InstallInstanceObjToCluster(testInstance, "default"); err != nil {
+			t.Fatal(err)
 		}
+		instanceList, err := getInstances(kc, env.DefaultSettings)
+		assert.NilError(t, err)
+		tassert.EqualValues(t, tt.instances, instanceList, "missing instances")
 	}
-	return diff
 }

pkg/kudoctl/cmd/init_test.go

@@ -67,7 +67,7 @@ func TestInitCmd_exists(t *testing.T) {
 		fs:     afero.NewMemMapFs(),
 		client: &kube.Client{KubeClient: fc, ExtClient: fc2},
 	}
-	clog.Init(nil, &buf)
+	clog.InitWithFlags(nil, &buf)
 	Settings.Home = "/opt"
 
 	if err := cmd.run(); err != nil {
@@ -160,13 +160,19 @@ func TestInitCmd_YAMLWriter(t *testing.T) {
 			"output":  "yaml",
 		}
 		for name, value := range flags {
-			initCmd.Flags().Set(name, value)
+			if err := initCmd.Flags().Set(name, value); err != nil {
+				t.Fatal(err)
+			}
 		}
 
 		for name, value := range tc.additionalFlags {
-			initCmd.Flags().Set(name, value)
+			if err := initCmd.Flags().Set(name, value); err != nil {
+				t.Fatal(err)
+			}
+		}
+		if err := initCmd.RunE(initCmd, []string{}); err != nil {
+			t.Fatal(err)
 		}
-		initCmd.RunE(initCmd, []string{})
 
 		gp := filepath.Join("testdata", tc.file+".golden")
 
@@ -181,7 +187,7 @@ func TestInitCmd_YAMLWriter(t *testing.T) {
 			t.Fatalf("failed reading .golden: %s", err)
 		}
 
-		assert.Equal(t, string(g), out.String())
+		assert.Equal(t, string(g), out.String(), "for golden file: %s", gp)
 	}
 }
 
@@ -194,9 +200,13 @@ func TestInitCmd_CustomNamespace(t *testing.T) {
 	flags := map[string]string{"dry-run": "true", "output": "yaml", "namespace": "foo"}
 
 	for flag, value := range flags {
-		initCmd.Flags().Set(flag, value)
+		if err := initCmd.Flags().Set(flag, value); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := initCmd.RunE(initCmd, []string{}); err != nil {
+		t.Fatal(err)
 	}
-	initCmd.RunE(initCmd, []string{})
 
 	gp := filepath.Join("testdata", file+".golden")
 
@@ -236,7 +246,9 @@ func TestNewInitCmd(t *testing.T) {
 			out := &bytes.Buffer{}
 			initCmd := newInitCmd(fs, out)
 			for key, value := range tt.flags {
-				initCmd.Flags().Set(key, value)
+				if err := initCmd.Flags().Set(key, value); err != nil {
+					t.Fatal(err)
+				}
 			}
 			err := initCmd.RunE(initCmd, tt.parameters)
 			assert.EqualError(t, err, tt.errorMessage)

pkg/kudoctl/cmd/install/install_test.go

@@ -2,6 +2,8 @@ package install
 
 import (
 	"testing"
+
+	"gotest.tools/assert"
 )
 
 func TestValidate(t *testing.T) {
@@ -17,10 +19,8 @@ func TestValidate(t *testing.T) {
 
 	for _, tt := range tests {
 		err := validate(tt.arg, DefaultOptions)
-		if err != nil {
-			if err.Error() != tt.err {
-				t.Errorf("Expecting error message '%s' but got '%s'", tt.err, err)
-			}
+		if tt.err != "" {
+			assert.ErrorContains(t, err, tt.err)
 		}
 	}
 }

pkg/kudoctl/cmd/install_test.go

@@ -44,7 +44,9 @@ func TestTableNewInstallCmd_WithParameters(t *testing.T) {
 	for _, test := range cmdParameterTests {
 		newCmdInstall := newInstallCmd(afero.NewOsFs())
 		for _, flag := range test.parameters {
-			newCmdInstall.Flags().Set("parameter", flag)
+			if err := newCmdInstall.Flags().Set("parameter", flag); err != nil {
+				t.Fatal(err)
+			}
 		}
 		err := newCmdInstall.RunE(newCmdInstall, []string{})
 		assert.NotNil(t, err, test.errorMessage)

pkg/kudoctl/cmd/package_test.go

@@ -24,7 +24,9 @@ var packageCmdArgs = []struct {
 func TestTableNewPackageCmd(t *testing.T) {
 	fs := afero.NewMemMapFs()
 	testdir, _ := filepath.Abs("")
-	fs.Mkdir(testdir, 0777)
+	if err := fs.Mkdir(testdir, 0777); err != nil {
+		t.Fatal(err)
+	}
 	files.CopyOperatorToFs(fs, "../packages/testdata/zk", "/opt")
 	for _, test := range packageCmdArgs {
 		newCmd := newPackageCmd(fs, os.Stdout)

pkg/kudoctl/cmd/repo_add_test.go

@@ -79,7 +79,9 @@ func TestAddSkipCheck(t *testing.T) {
 	// verified through repo list golden file
 	out = &bytes.Buffer{}
 	rl := &repoListCmd{out: out, home: home}
-	rl.run(fs)
+	if err := rl.run(fs); err != nil {
+		t.Fatal(err)
+	}
 	gp := filepath.Join("testdata", file+".golden")
 
 	if *updateGolden {

pkg/kudoctl/cmd/repo_index_test.go

@@ -34,7 +34,9 @@ func TestRepoIndexCmd(t *testing.T) {
 			time := time.Now()
 			riCmd := newRepoIndexCmd(fs, out, &time)
 			for key, value := range tt.flags {
-				riCmd.Flags().Set(key, value)
+				if err := riCmd.Flags().Set(key, value); err != nil {
+					t.Fatal(err)
+				}
 			}
 			err := riCmd.RunE(riCmd, tt.arguments)
 			assert.EqualError(t, err, tt.errorMessage)
@@ -46,13 +48,17 @@ func TestRepoIndexCmd_IndexCreation(t *testing.T) {
 	file := "index.yaml"
 	fs := afero.NewMemMapFs()
 	testdir, _ := filepath.Abs("")
-	fs.Mkdir(testdir, 0777)
+	if err := fs.Mkdir(testdir, 0777); err != nil {
+		t.Fatal(err)
+	}
 	files.CopyOperatorToFs(fs, "../packages/testdata/zk.tgz", "/opt")
 
 	time, _ := time.Parse(time.RFC3339, "2019-10-25T00:00:00Z")
 	out := &bytes.Buffer{}
 	riCmd := newRepoIndexCmd(fs, out, &time)
-	riCmd.RunE(riCmd, []string{"/opt"})
+	if err := riCmd.RunE(riCmd, []string{"/opt"}); err != nil {
+		t.Fatal(err)
+	}
 
 	indexOut, err := afero.ReadFile(fs, "/opt/index.yaml")
 	if err != nil {
@@ -72,9 +78,6 @@ func TestRepoIndexCmd_IndexCreation(t *testing.T) {
 	}
 
 	assert.Equal(t, string(indexOut), string(g), "yaml does not match .golden file %s", gp)
-	//if !bytes.Equal(indexOut, g) {
-	//	t.Errorf("yaml does not match .golden file %s:\n%s", gp, string(indexOut))
-	//}
 }
 
 func TestRepoIndexCmd_MergeIndex(t *testing.T) {
@@ -88,7 +91,9 @@ func TestRepoIndexCmd_MergeIndex(t *testing.T) {
 
 	resultBuf := &bytes.Buffer{}
 	merge(indexFile, mergeFile)
-	indexFile.Write(resultBuf)
+	if err := indexFile.Write(resultBuf); err != nil {
+		t.Fatal(err)
+	}
 
 	gp := filepath.Join("testdata", file+".golden")
 

pkg/kudoctl/cmd/repo_list_test.go

@@ -31,7 +31,9 @@ func TestRepoList(t *testing.T) {
 	// reset buffer for repo list
 	out = &bytes.Buffer{}
 	rl := &repoListCmd{out: out, home: home}
-	rl.run(fs)
+	if err := rl.run(fs); err != nil {
+		t.Fatal(err)
+	}
 	gp := filepath.Join("testdata", file+".golden")
 
 	if *updateGolden {

pkg/kudoctl/cmd/root.go

@@ -2,7 +2,6 @@ package cmd
 
 import (
 	"io"
-	"os"
 
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
@@ -74,9 +73,5 @@ and serves as an API aggregation layer.
 func initGlobalFlags(cmd *cobra.Command, out io.Writer) {
 	flags := cmd.PersistentFlags()
 	Settings.AddFlags(flags)
-	clog.Init(flags, out)
-	// FIXME: add error handling
-	cmd.ParseFlags(os.Args[1:])
-	// set ENV if flags are not used.
-	Settings.Init(flags)
+	clog.InitWithFlags(flags, out)
 }