Add Linter (hashicorp#328)

* add a linter, and add a lint check to the makefile, remove any existing lint violations

.golangci-lint.yml

@@ -0,0 +1,49 @@
+run:
+  deadline: 5m
+
+linters-settings:
+  govet:
+    check-shadowing: true
+  golint:
+    min-confidence: 0
+
+linters:
+  disable-all: true
+  enable:
+    - gofmt
+    #- golint
+    - govet
+    #- varcheck
+    #- typecheck
+    #- gosimple
+
+issues:
+  exclude-use-default: false
+  exclude:
+      # ignore the false positive erros resulting from not including a comment above every `package` keyword
+    - should have a package comment, unless it's in another file for this package (golint)
+      # golint: Annoying issue about not having a comment. The rare codebase has such comments
+      # - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)
+      # errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
+    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked
+
+      # golint: False positive when tests are defined in package 'test'
+    - func name will be used as test\.Test.* by other packages, and that stutters; consider calling this
+
+      # staticcheck: Developers tend to write in C-style with an 
+      # explicit 'break' in a 'switch', so it's ok to ignore
+    - ineffective break statement. Did you mean to break out of the outer loop
+      # gosec: Too many false-positives on 'unsafe' usage
+    - Use of unsafe calls should be audited
+
+    # gosec: Too many false-positives for parametrized shell calls
+    - Subprocess launch(ed with variable|ing should be audited)
+
+    # gosec: Duplicated errcheck checks
+    - G104
+
+    # gosec: Too many issues in popular repos
+    - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
+
+    # gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
+    - Potential file inclusion via variable

.travis.yml

@@ -8,7 +8,10 @@ go:
     - 1.12
     - tip
 
-install: make deps
+install: 
+    - make deps 
+    - curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin latest 
+
 script:
     - make integ
 

Makefile

@@ -1,4 +1,18 @@
 DEPS = $(go list -f '{{range .TestImports}}{{.}} {{end}}' ./...)
+ENV  = $(shell go env GOPATH)
+GO_VERSION  = $(shell go version)
+GOLANG_CI_VERSION = v1.19.0
+
+# Look for versions prior to 1.10 which have a different fmt output
+# and don't lint with gofmt against them.
+ifneq (,$(findstring go version go1.8, $(GO_VERSION)))
+	FMT=
+else ifneq (,$(findstring go version go1.9, $(GO_VERSION)))
+	FMT=
+else
+    FMT=--enable gofmt
+endif
+
 TEST_RESULTS_DIR?=/tmp/test-results
 
 test:
@@ -29,8 +43,15 @@ deps:
 	go get -t -d -v ./...
 	echo $(DEPS) | xargs -n1 go get -d
 
+lint:
+	gofmt -s -w .
+	golangci-lint run -c .golangci-lint.yml $(FMT) .
+
+dep-linter:
+	curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(ENV)/bin $(GOLANG_CI_VERSION)
+
 cov:
 	INTEG_TESTS=yes gocov test github.com/hashicorp/raft | gocov-html > /tmp/coverage.html
 	open /tmp/coverage.html
 
-.PHONY: test cov integ deps
+.PHONY: test cov integ deps dep-linter lint

api.go

@@ -9,9 +9,8 @@ import (
 	"sync"
 	"time"
 
-	"github.com/hashicorp/go-hclog"
-
-	"github.com/armon/go-metrics"
+	metrics "github.com/armon/go-metrics"
+	hclog "github.com/hashicorp/go-hclog"
 )
 
 const (
@@ -288,37 +287,36 @@ func RecoverCluster(conf *Config, fsm FSM, logs LogStore, stable StableStore,
 	// expect data to be there and it's not. By refusing, we force them
 	// to show intent to start a cluster fresh by explicitly doing a
 	// bootstrap, rather than quietly fire up a fresh cluster here.
-	hasState, err := HasExistingState(logs, stable, snaps)
-	if err != nil {
+	if hasState, err := HasExistingState(logs, stable, snaps); err != nil {
 		return fmt.Errorf("failed to check for existing state: %v", err)
-	}
-	if !hasState {
+	} else if !hasState {
 		return fmt.Errorf("refused to recover cluster with no initial state, this is probably an operator error")
 	}
 
 	// Attempt to restore any snapshots we find, newest to oldest.
-	var snapshotIndex uint64
-	var snapshotTerm uint64
-	snapshots, err := snaps.List()
+	var (
+		snapshotIndex  uint64
+		snapshotTerm   uint64
+		snapshots, err = snaps.List()
+	)
 	if err != nil {
 		return fmt.Errorf("failed to list snapshots: %v", err)
 	}
 	for _, snapshot := range snapshots {
-		if !conf.NoSnapshotRestoreOnStart {
-			_, source, err := snaps.Open(snapshot.ID)
-			if err != nil {
-				// Skip this one and try the next. We will detect if we
-				// couldn't open any snapshots.
-				continue
-			}
+		var source io.ReadCloser
+		_, source, err = snaps.Open(snapshot.ID)
+		if err != nil {
+			// Skip this one and try the next. We will detect if we
+			// couldn't open any snapshots.
+			continue
+		}
 
-			err = fsm.Restore(source)
-			// Close the source after the restore has completed
-			source.Close()
-			if err != nil {
-				// Same here, skip and try the next one.
-				continue
-			}
+		err = fsm.Restore(source)
+		// Close the source after the restore has completed
+		source.Close()
+		if err != nil {
+			// Same here, skip and try the next one.
+			continue
 		}
 
 		snapshotIndex = snapshot.Index
@@ -341,7 +339,7 @@ func RecoverCluster(conf *Config, fsm FSM, logs LogStore, stable StableStore,
 	}
 	for index := snapshotIndex + 1; index <= lastLogIndex; index++ {
 		var entry Log
-		if err := logs.GetLog(index, &entry); err != nil {
+		if err = logs.GetLog(index, &entry); err != nil {
 			return fmt.Errorf("failed to get log at index %d: %v", index, err)
 		}
 		if entry.Type == LogCommand {
@@ -362,10 +360,10 @@ func RecoverCluster(conf *Config, fsm FSM, logs LogStore, stable StableStore,
 	if err != nil {
 		return fmt.Errorf("failed to create snapshot: %v", err)
 	}
-	if err := snapshot.Persist(sink); err != nil {
+	if err = snapshot.Persist(sink); err != nil {
 		return fmt.Errorf("failed to persist snapshot: %v", err)
 	}
-	if err := sink.Close(); err != nil {
+	if err = sink.Close(); err != nil {
 		return fmt.Errorf("failed to finalize snapshot: %v", err)
 	}
 

bench/bench.go

@@ -80,9 +80,9 @@ func StoreLogs(b *testing.B, store raft.LogStore) {
 		b.StopTimer()
 		offset := 3 * (n + 1)
 		logs := []*raft.Log{
-			&raft.Log{Index: uint64(offset - 2), Data: []byte("data")},
-			&raft.Log{Index: uint64(offset - 1), Data: []byte("data")},
-			&raft.Log{Index: uint64(offset), Data: []byte("data")},
+			{Index: uint64(offset - 2), Data: []byte("data")},
+			{Index: uint64(offset - 1), Data: []byte("data")},
+			{Index: uint64(offset), Data: []byte("data")},
 		}
 		b.StartTimer()
 

configuration_test.go

@@ -9,17 +9,17 @@ import (
 
 var sampleConfiguration = Configuration{
 	Servers: []Server{
-		Server{
+		{
 			Suffrage: Nonvoter,
 			ID:       ServerID("id0"),
 			Address:  ServerAddress("addr0"),
 		},
-		Server{
+		{
 			Suffrage: Voter,
 			ID:       ServerID("id1"),
 			Address:  ServerAddress("addr1"),
 		},
-		Server{
+		{
 			Suffrage: Staging,
 			ID:       ServerID("id2"),
 			Address:  ServerAddress("addr2"),
@@ -39,20 +39,20 @@ func TestConfiguration_Configuration_Clone(t *testing.T) {
 }
 
 func TestConfiguration_configurations_Clone(t *testing.T) {
-	configurations := configurations{
+	configuration := configurations{
 		committed:      sampleConfiguration,
 		committedIndex: 1,
 		latest:         sampleConfiguration,
 		latestIndex:    2,
 	}
-	cloned := configurations.Clone()
-	if !reflect.DeepEqual(configurations, cloned) {
-		t.Fatalf("mismatch %v %v", configurations, cloned)
+	cloned := configuration.Clone()
+	if !reflect.DeepEqual(configuration, cloned) {
+		t.Fatalf("mismatch %v %v", configuration, cloned)
 	}
 	cloned.committed.Servers[1].ID = "scribble"
 	cloned.latest.Servers[1].ID = "scribble"
-	if configurations.committed.Servers[1].ID == "scribble" ||
-		configurations.latest.Servers[1].ID == "scribble" {
+	if configuration.committed.Servers[1].ID == "scribble" ||
+		configuration.latest.Servers[1].ID == "scribble" {
 		t.Fatalf("cloned configuration shouldn't alias Servers")
 	}
 }
@@ -118,7 +118,7 @@ func TestConfiguration_checkConfiguration(t *testing.T) {
 
 var singleServer = Configuration{
 	Servers: []Server{
-		Server{
+		{
 			Suffrage: Voter,
 			ID:       ServerID("id1"),
 			Address:  ServerAddress("addr1x"),
@@ -128,17 +128,17 @@ var singleServer = Configuration{
 
 var oneOfEach = Configuration{
 	Servers: []Server{
-		Server{
+		{
 			Suffrage: Voter,
 			ID:       ServerID("id1"),
 			Address:  ServerAddress("addr1x"),
 		},
-		Server{
+		{
 			Suffrage: Staging,
 			ID:       ServerID("id2"),
 			Address:  ServerAddress("addr2x"),
 		},
-		Server{
+		{
 			Suffrage: Nonvoter,
 			ID:       ServerID("id3"),
 			Address:  ServerAddress("addr3x"),
@@ -148,12 +148,12 @@ var oneOfEach = Configuration{
 
 var voterPair = Configuration{
 	Servers: []Server{
-		Server{
+		{
 			Suffrage: Voter,
 			ID:       ServerID("id1"),
 			Address:  ServerAddress("addr1x"),
 		},
-		Server{
+		{
 			Suffrage: Voter,
 			ID:       ServerID("id2"),
 			Address:  ServerAddress("addr2x"),

file_snapshot.go

@@ -487,9 +487,11 @@ func (s *FileSnapshotSink) finalize() error {
 
 // writeMeta is used to write out the metadata we have.
 func (s *FileSnapshotSink) writeMeta() error {
+	var err error
 	// Open the meta file
 	metaPath := filepath.Join(s.dir, metaFilePath)
-	fh, err := os.Create(metaPath)
+	var fh *os.File
+	fh, err = os.Create(metaPath)
 	if err != nil {
 		return err
 	}
@@ -500,7 +502,7 @@ func (s *FileSnapshotSink) writeMeta() error {
 
 	// Write out as JSON
 	enc := json.NewEncoder(buffered)
-	if err := enc.Encode(&s.meta); err != nil {
+	if err = enc.Encode(&s.meta); err != nil {
 		return err
 	}
 

file_snapshot_test.go

@@ -194,22 +194,26 @@ func TestFileSS_CancelSnapshot(t *testing.T) {
 }
 
 func TestFileSS_Retention(t *testing.T) {
+	var err error
 	// Create a test dir
-	dir, err := ioutil.TempDir("", "raft")
+	var dir string
+	dir, err = ioutil.TempDir("", "raft")
 	if err != nil {
 		t.Fatalf("err: %v ", err)
 	}
 	defer os.RemoveAll(dir)
 
-	snap, err := NewFileSnapshotStoreWithLogger(dir, 2, newTestLogger(t))
+	var snap *FileSnapshotStore
+	snap, err = NewFileSnapshotStoreWithLogger(dir, 2, newTestLogger(t))
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
 
 	// Create a few snapshots
 	_, trans := NewInmemTransport(NewInmemAddr())
 	for i := 10; i < 15; i++ {
-		sink, err := snap.Create(SnapshotVersionMax, uint64(i), 3, Configuration{}, 0, trans)
+		var sink SnapshotSink
+		sink, err = snap.Create(SnapshotVersionMax, uint64(i), 3, Configuration{}, 0, trans)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
@@ -220,7 +224,8 @@ func TestFileSS_Retention(t *testing.T) {
 	}
 
 	// Should only have 2 listed!
-	snaps, err := snap.List()
+	var snaps []*SnapshotMeta
+	snaps, err = snap.List()
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -238,29 +243,32 @@ func TestFileSS_Retention(t *testing.T) {
 }
 
 func TestFileSS_BadPerm(t *testing.T) {
+	var err error
 	if runtime.GOOS == "windows" {
 		t.Skip("skipping file permission test on windows")
 	}
 
 	// Create a temp dir
-	dir1, err := ioutil.TempDir("", "raft")
+	var dir1 string
+	dir1, err = ioutil.TempDir("", "raft")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
 	defer os.RemoveAll(dir1)
 
 	// Create a sub dir and remove all permissions
-	dir2, err := ioutil.TempDir(dir1, "badperm")
+	var dir2 string
+	dir2, err = ioutil.TempDir(dir1, "badperm")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	if err := os.Chmod(dir2, 000); err != nil {
+	if err = os.Chmod(dir2, 000); err != nil {
 		t.Fatalf("err: %s", err)
 	}
 	defer os.Chmod(dir2, 777) // Set perms back for delete
 
 	// Should fail
-	if _, err := NewFileSnapshotStore(dir2, 3, nil); err == nil {
+	if _, err = NewFileSnapshotStore(dir2, 3, nil); err == nil {
 		t.Fatalf("should fail to use dir with bad perms")
 	}
 }