add gometaliner into travis build (#254)

and gradually tighten the linting rules. for now unused and golint are left out. some fixes are included in this PR. using gometaliner instead of just golint because: it covers more cases, e.g. the typo checkings. it also allows for disabling rules w.r.t regex, e.g. when you don't want to enforce all the comment rules enforced by golint * Part of #53
kubeflow · Dec 29, 2017 · 433b234 · 433b234
1 parent 420f9aa
commit 433b234
Show file tree

Hide file tree

Showing 14 changed files with 74 additions and 32 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -1,14 +1,23 @@
 language: go
 
 go:
+  # we cannot enable `gotyp` in gometalinter while at the same time
+  # keep 1.8 here. so I (jiay) went with removing `gotype` for now.
+  # see open issues:
+  # - https://github.com/alecthomas/gometalinter/issues/358
+  # - https://github.com/golang/go/issues/21712
+  # TODO jiayu: re-evaluate the pending issues and add back `gotype`
   - 1.8
   - 1.9
 
 install:
   # get coveralls.io support
   - go get github.com/mattn/goveralls
+  - go get -u github.com/alecthomas/gometalinter
+  - gometalinter --install
 
 script:
+  - gometalinter --config=linter_config.json ./pkg/...
   # We customize the build step because by default
   # Travis runs go test -v ./... which will include the vendor
   # directory.

diff --git a/README.md b/README.md
@@ -1,11 +1,9 @@
 # K8s Custom Resource and Operator For TensorFlow jobs
 
 [![Build Status](https://travis-ci.org/tensorflow/k8s.svg?branch=master)](https://travis-ci.org/tensorflow/k8s)
-
 [![Coverage Status](https://coveralls.io/repos/github/tensorflow/k8s/badge.svg?branch=master)](https://coveralls.io/github/tensorflow/k8s?branch=master)
-
+[![Go Report Card](https://goreportcard.com/badge/github.com/tensorflow/k8s)](https://goreportcard.com/report/github.com/tensorflow/k8s)
 [Prow Test Dashboard](https://k8s-testgrid.appspot.com/sig-big-data)
-
 [Prow Jobs](https://prow.k8s.io/?repo=tensorflow%2Fk8s)
 
 ## Overview

diff --git a/linter_config.json b/linter_config.json
@@ -0,0 +1,25 @@
+{
+  "Vendor": true,
+  "DisableAll": true,
+  "Enable": [
+    "vet",
+    "safesql",
+    "errcheck",
+    "goconst",
+    "goimports",
+    "varcheck",
+    "gas",
+    "staticcheck",
+    "gosimple",
+    "lll",
+    "unconvert",
+    "misspell",
+    "unconvert"
+  ],
+  "Aggregate": true,
+  "WarnUnmatchedNolint": true,
+  "LineLength": 240,
+  "Exclude": ["comment or be unexported", "comment on exported"],
+  "Deadline": "300s",
+  "Skip": ["bin", "py"]
+}
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -304,7 +304,9 @@ func (c *Controller) watch(watchVersion string) (<-chan *Event, <-chan error) {
 			}
 			if resp.StatusCode != http.StatusOK {
 				log.Infof("WatchClusters response: %+v", resp)
-				resp.Body.Close()
+				if err := resp.Body.Close(); err != nil {
+					log.Errorf("error closing response body: %v", err)
+				}
 				errCh <- errors.New("invalid status code: " + resp.Status)
 				return
 			}
@@ -326,7 +328,9 @@ func (c *Controller) watch(watchVersion string) (<-chan *Event, <-chan error) {
 				}
 
 				if st != nil {
-					resp.Body.Close()
+					if err := resp.Body.Close(); err != nil {
+						log.Errorf("error closing response body: %v", err)
+					}
 
 					if st.Code == http.StatusGone {
 						// event history is outdated.
@@ -353,7 +357,9 @@ func (c *Controller) watch(watchVersion string) (<-chan *Event, <-chan error) {
 				eventCh <- ev
 			}
 
-			resp.Body.Close()
+			if err := resp.Body.Close(); err != nil {
+				log.Errorf("error closing response body: %v", err)
+			}
 		}
 	}()
 

diff --git a/pkg/spec/tf_job.go b/pkg/spec/tf_job.go
@@ -127,7 +127,7 @@ type ChiefSpec struct {
 
 // Validate checks that the TfJobSpec is valid.
 func (c *TfJobSpec) Validate() error {
-	if c.TerminationPolicy == nil || c.TerminationPolicy.Chief == nil  {
+	if c.TerminationPolicy == nil || c.TerminationPolicy.Chief == nil {
 		return fmt.Errorf("invalid termination policy: %v", c.TerminationPolicy)
 	}
 	// Check that each replica has a TensorFlow container.

diff --git a/pkg/spec/tf_job_test.go b/pkg/spec/tf_job_test.go
@@ -386,7 +386,7 @@ func TestSetDefaults(t *testing.T) {
 
 func TestValidate(t *testing.T) {
 	type testCase struct {
-		in       *TfJobSpec
+		in             *TfJobSpec
 		expectingError bool
 	}
 
@@ -405,7 +405,7 @@ func TestValidate(t *testing.T) {
 							},
 						},
 						TfReplicaType: MASTER,
-						Replicas: proto.Int32(1),
+						Replicas:      proto.Int32(1),
 					},
 				},
 				TfImage: "tensorflow/tensorflow:1.3.0",
@@ -426,7 +426,7 @@ func TestValidate(t *testing.T) {
 							},
 						},
 						TfReplicaType: WORKER,
-						Replicas: proto.Int32(1),
+						Replicas:      proto.Int32(1),
 					},
 				},
 				TfImage: "tensorflow/tensorflow:1.3.0",
@@ -447,13 +447,13 @@ func TestValidate(t *testing.T) {
 							},
 						},
 						TfReplicaType: WORKER,
-						Replicas: proto.Int32(1),
+						Replicas:      proto.Int32(1),
 					},
 				},
 				TfImage: "tensorflow/tensorflow:1.3.0",
 				TerminationPolicy: &TerminationPolicySpec{
 					Chief: &ChiefSpec{
-						ReplicaName: "WORKER",
+						ReplicaName:  "WORKER",
 						ReplicaIndex: 0,
 					},
 				},

diff --git a/pkg/trainer/replicas.go b/pkg/trainer/replicas.go
@@ -110,9 +110,7 @@ func transformClusterSpecForDefaultPS(clusterSpec ClusterSpec) string {
 	jobs := []string{}
 	for _, jobType := range keys {
 		hosts := []string{}
-		for _, h := range clusterSpec[jobType] {
-			hosts = append(hosts, h)
-		}
+		hosts = append(hosts, clusterSpec[jobType]...)
 		s := jobType + "|" + strings.Join(hosts, ";")
 		jobs = append(jobs, s)
 	}
@@ -341,7 +339,9 @@ func (s *TFReplicaSet) Delete() error {
 		}
 	} else {
 		log.V(1).Infof("Delete ConfigMaps %v:%v", s.Job.job.Metadata.Namespace, s.defaultPSConfigMapName())
-		s.ClientSet.CoreV1().ConfigMaps(s.Job.job.Metadata.Namespace).Delete(s.defaultPSConfigMapName(), &meta_v1.DeleteOptions{})
+		if err = s.ClientSet.CoreV1().ConfigMaps(s.Job.job.Metadata.Namespace).Delete(s.defaultPSConfigMapName(), &meta_v1.DeleteOptions{}); err != nil {
+			log.Errorf("error deleting config maps %v:%v: %v", s.Job.job.Metadata.Namespace, s.defaultPSConfigMapName(), err)
+		}
 	}
 
 	if failures {
@@ -406,7 +406,7 @@ func replicaStatusFromPodList(l v1.PodList, name spec.ContainerName) spec.Replic
 	return spec.ReplicaStateUnknown
 }
 
-func (s *TFReplicaSet) GetSingleReplicaStatus(index int32) (spec.ReplicaState) {
+func (s *TFReplicaSet) GetSingleReplicaStatus(index int32) spec.ReplicaState {
 	j, err := s.ClientSet.BatchV1().Jobs(s.Job.job.Metadata.Namespace).Get(s.jobName(index), meta_v1.GetOptions{})
 
 	if err != nil {

diff --git a/pkg/trainer/replicas_test.go b/pkg/trainer/replicas_test.go
@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"encoding/json"
+
 	"github.com/tensorflow/k8s/pkg/spec"
 	"github.com/tensorflow/k8s/pkg/util"
 	tfJobFake "github.com/tensorflow/k8s/pkg/util/k8sutil/fake"

diff --git a/pkg/trainer/tensorboard.go b/pkg/trainer/tensorboard.go
@@ -153,18 +153,14 @@ func (s *TBReplicaSet) getDeploymentSpecTemplate(image string) v1.PodTemplateSpe
 		VolumeMounts: make([]v1.VolumeMount, 0),
 	}
 
-	for _, v := range s.Spec.VolumeMounts {
-		c.VolumeMounts = append(c.VolumeMounts, v)
-	}
+	c.VolumeMounts = append(c.VolumeMounts, s.Spec.VolumeMounts...)
 
 	ps := &v1.PodSpec{
 		Containers: []v1.Container{*c},
 		Volumes:    make([]v1.Volume, 0),
 	}
 
-	for _, v := range s.Spec.Volumes {
-		ps.Volumes = append(ps.Volumes, v)
-	}
+	ps.Volumes = append(ps.Volumes, s.Spec.Volumes...)
 
 	return v1.PodTemplateSpec{
 		ObjectMeta: meta_v1.ObjectMeta{

diff --git a/pkg/trainer/training.go b/pkg/trainer/training.go
@@ -65,7 +65,7 @@ type TrainingJob struct {
 
 // ClusterSpec represents a cluster TensorFlow specification.
 // https://www.tensorflow.org/deploy/distributed#create_a_tftrainclusterspec_to_describe_the_cluster
-// It is a map from job names to network addressess.
+// It is a map from job names to network addresses.
 type ClusterSpec map[string][]string
 
 type TaskSpec struct {
@@ -179,7 +179,7 @@ func (j *TrainingJob) GetStatus() (spec.ReplicaState, []*spec.TfReplicaStatus, e
 
 		replicaStatuses = append(replicaStatuses, &rStatus)
 
-		if string(r.Spec.TfReplicaType) == string(chief.ReplicaName) {
+		if string(r.Spec.TfReplicaType) == chief.ReplicaName {
 			chiefState = r.GetSingleReplicaStatus(int32(chief.ReplicaIndex))
 		}
 	}
@@ -390,7 +390,7 @@ func (j *TrainingJob) reconcile(config *spec.ControllerConfig) {
 	}
 
 	// updateTPRStatus will update the status of the TPR with c.Status if c.Status
-	// doesn't match c.Cluster.status. So you can change c.Status in order to propogate
+	// doesn't match c.Cluster.status. So you can change c.Status in order to propagate
 	// changes to the TPR status.
 	if err := j.updateTPRStatus(); err != nil {
 		log.Warningf("Job %v; failed to update TPR status error: %v", j.job.Metadata.Name, err)

diff --git a/pkg/trainer/training_test.go b/pkg/trainer/training_test.go
@@ -4,14 +4,15 @@ import (
 	"reflect"
 	"testing"
 
+	"sync"
+
 	"github.com/gogo/protobuf/proto"
 	"github.com/tensorflow/k8s/pkg/spec"
 	tfJobFake "github.com/tensorflow/k8s/pkg/util/k8sutil/fake"
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
-	"sync"
 )
 
 func TestIsRetryableTerminationState(t *testing.T) {

diff --git a/pkg/util/k8sutil/fake/fake.go b/pkg/util/k8sutil/fake/fake.go
@@ -2,9 +2,10 @@
 package fake
 
 import (
-	"github.com/tensorflow/k8s/pkg/spec"
 	"net/http"
 	"time"
+
+	"github.com/tensorflow/k8s/pkg/spec"
 )
 
 type TfJobClientFake struct{}

diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go
@@ -6,9 +6,9 @@ import (
 
 	"github.com/tensorflow/k8s/pkg/spec"
 
+	log "github.com/golang/glog"
 	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	log "github.com/golang/glog"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/kubernetes"
@@ -56,10 +56,14 @@ func GetClusterConfig() (*rest.Config, error) {
 		if err != nil {
 			panic(err)
 		}
-		os.Setenv("KUBERNETES_SERVICE_HOST", addrs[0])
+		if err := os.Setenv("KUBERNETES_SERVICE_HOST", addrs[0]); err != nil {
+			return nil, err
+		}
 	}
 	if len(os.Getenv("KUBERNETES_SERVICE_PORT")) == 0 {
-		os.Setenv("KUBERNETES_SERVICE_PORT", "443")
+		if err := os.Setenv("KUBERNETES_SERVICE_PORT", "443"); err != nil {
+			panic(err)
+		}
 	}
 	return rest.InClusterConfig()
 }

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -4,9 +4,10 @@ package util
 import (
 	"encoding/json"
 	"fmt"
-	log "github.com/golang/glog"
 	"math/rand"
 	"time"
+
+	log "github.com/golang/glog"
 )
 
 // Pformat returns a pretty format output of any value that can be marshalled to JSON.