Enabled golinter `errcheck` option #1014

zen-dog · 2019-10-29T17:40:52Z

and fixed all the resulting linter errors.

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

and fixed all the resulting linter errors.

zen-dog · 2019-10-29T17:43:23Z

pkg/kudoctl/cmd/get/get_test.go

-		{[]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"}},


All of these test cases already exist above in TestValidate and were effectively a no-op here.

zen-dog · 2019-10-29T17:44:58Z

pkg/kudoctl/cmd/install/install_test.go

-			if err.Error() != tt.err {
-				t.Errorf("Expecting error message '%s' but got '%s'", tt.err, err)
-			}
+		if len(tt.err) > 0 {


This is a pattern that we use in a few places and it is wrong: on the left, we fail to test the case where the resulting err == nil but should be something else.

Nit: Can we test for != "" here instead? I associate len with an array and while a string is an array of chars, this isn't the first thing that comes to mind when seeing this

Suggested change

if len(tt.err) > 0 {

if tt.err != "" {

and fixed `kubeconfig` location for `windows`

zen-dog · 2019-10-29T18:32:55Z

pkg/kudoctl/cmd/update_test.go

@@ -17,27 +18,24 @@ func TestUpdateCommand_Validation(t *testing.T) {
 		name         string
 		args         []string
 		instanceName string
-		parameters   map[string]string


parameters were effectively not tested here.

zen-dog · 2019-10-29T18:34:39Z

pkg/kudoctl/cmd/root.go

@@ -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:])


This ParseFlags() call had no effect. Env var overrides are now taken directly as defaults when adding the cobra flag.

zen-dog · 2019-10-29T18:35:35Z

pkg/kudoctl/env/environment.go

 // AddFlags binds flags to the given flagset.
 func (s *Settings) AddFlags(fs *pflag.FlagSet) {
-	fs.StringVar((*string)(&s.Home), "home", DefaultKudoHome, "location of your KUDO config.")
-	fs.StringVar(&s.KubeConfig, "kubeconfig", os.Getenv("HOME")+"/.kube/config", "Path to your Kubernetes configuration file.")


kubeconfig location on GOOS == "windows" was broken since there is no os.Getenv("HOME") there.

zen-dog · 2019-10-29T18:39:05Z

pkg/test/utils/kubernetes.go

@@ -584,36 +590,48 @@ func WithNamespace(obj runtime.Object, namespace string) runtime.Object {
 }

 // WithSpec applies the provided spec to the Kubernetes object.
-func WithSpec(obj runtime.Object, spec map[string]interface{}) runtime.Object {
-	return WithKeyValue(obj, "spec", spec)
+func WithSpec(t *testing.T, obj runtime.Object, spec map[string]interface{}) runtime.Object {


I'm not very proud of this solution: passing in t *testing.T and failing the test here. But, the way WithSpec, WithStatus and WithLabels methods are used to construct testing tables, makes it hard to do anything else meaningfully. At least we actually fail now if something goes wrong here.

zen-dog · 2019-10-29T18:40:34Z

pkg/test/utils/kubernetes.go

 }

 // WithKeyValue sets key in the provided object to value.
-func WithKeyValue(obj runtime.Object, key string, value map[string]interface{}) runtime.Object {
+func WithKeyValue(obj runtime.Object, key string, value map[string]interface{}) (runtime.Object, error) {


This was fun: we would ignore two ways this method could fail return the copy of the passed object instead. Which does not make the debugging of tests using this method easy.

zen-dog · 2019-10-29T18:41:47Z

pkg/kudoctl/env/environment.go

 // AddFlags binds flags to the given flagset.
 func (s *Settings) AddFlags(fs *pflag.FlagSet) {
-	fs.StringVar((*string)(&s.Home), "home", DefaultKudoHome, "location of your KUDO config.")
-	fs.StringVar(&s.KubeConfig, "kubeconfig", os.Getenv("HOME")+"/.kube/config", "Path to your Kubernetes configuration file.")
+	fs.StringVar((*string)(&s.Home), "home", kudoHome(), "location of your KUDO config.")


The env vars overrides (if exist) are used as the default for the cobra flag.

gerred · 2019-10-29T18:45:56Z

This is a huge improvement, well done. It's nice that we can be much more confident in our tests and everything as a whole. This is such a far-reaching PR that there can always be other refactoring to do, but want to keep this specific to errcheck unless there was anything glaring - and there's not. 👍

nfnt

Looks great, thanks for fixing all these missing cases and enabled errcheck! Just a few comments regarding the use of an additional assertion library. testify, which is already used, should be able to do this as well.

pkg/kudoctl/cmd/get/get_test.go

pkg/kudoctl/cmd/install/install_test.go

alenkacz

Loving this cleanup, thanks!

pkg/controller/instance/instance_controller_suite_test.go

pkg/kudoctl/cmd/get/get_test.go

alenkacz · 2019-10-30T14:22:55Z

pkg/kudoctl/env/environment.go

 var DefaultKudoHome = filepath.Join(homedir.HomeDir(), ".kudo")
+var DefaultKubeConfig = filepath.Join(homedir.HomeDir(), "/.kube/config")
+
+func kudoHome() string {


Enabled golinter errcheck option

2aba4d8

and fixed all the resulting linter errors.

zen-dog requested review from kensipe, alenkacz, gerred and nfnt October 29, 2019 17:40

zen-dog requested a review from jbarrick-mesosphere as a code owner October 29, 2019 17:40

Goland is too smart sometimes

5fd5637

zen-dog commented Oct 29, 2019

View reviewed changes

Simplified env var overrides for .kudo and kubeconfig

e214c78

and fixed `kubeconfig` location for `windows`

zen-dog commented Oct 29, 2019

View reviewed changes

gerred approved these changes Oct 29, 2019

View reviewed changes

Review feedback

49a55db

nfnt requested changes Oct 30, 2019

View reviewed changes

pkg/kudoctl/cmd/get/get_test.go Show resolved Hide resolved

pkg/kudoctl/cmd/install/install_test.go Show resolved Hide resolved

nfnt approved these changes Oct 30, 2019

View reviewed changes

alenkacz approved these changes Oct 30, 2019

View reviewed changes

More review feedback

545b2f4

zen-dog merged commit 95877c1 into master Oct 30, 2019

zen-dog deleted the ad/enable-errcheck branch October 30, 2019 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled golinter `errcheck` option #1014

Enabled golinter `errcheck` option #1014

zen-dog commented Oct 29, 2019 •

edited

Loading

zen-dog Oct 29, 2019

zen-dog Oct 29, 2019

nfnt Oct 30, 2019

zen-dog Oct 29, 2019

zen-dog Oct 29, 2019

zen-dog Oct 29, 2019

zen-dog Oct 29, 2019 •

edited

Loading

zen-dog Oct 29, 2019

zen-dog Oct 29, 2019

gerred commented Oct 29, 2019

nfnt left a comment

alenkacz left a comment

alenkacz Oct 30, 2019

Enabled golinter errcheck option #1014

Enabled golinter errcheck option #1014

Conversation

zen-dog commented Oct 29, 2019 • edited Loading

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

zen-dog Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerred commented Oct 29, 2019

nfnt left a comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enabled golinter `errcheck` option #1014

Enabled golinter `errcheck` option #1014

zen-dog commented Oct 29, 2019 •

edited

Loading

zen-dog Oct 29, 2019 •

edited

Loading