Repository: allegro/marathon-consul
Pull request #252 (2017-07-22 16:17:33)

Enable gometalinter.
It looks like linter was only enabled on top level code
non of the packages were scannded. This pull request
enables linter on `./...` and fixes reported issues.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/12507063/badge)](https://coveralls.io/builds/12507063)

Coverage increased (+0.02%) to 89.391% when pulling **a306d42e2398858ca1723968d1371eeec5d44a40 on enable_linter_to_whole_project** into **1cda5744ffbaa7c1b67e44ed547fd5bd58af6eeb on master**.


Patch:
diff --git a/Makefile b/Makefile
index f95b79b..0e645b8 100644
--- a/Makefile
+++ b/Makefile
@@ -55,7 +55,7 @@ check-deps: deps
         (go get github.com/alecthomas/gometalinter && gometalinter --install)
 
 check: check-deps $(SOURCES) test
-	gometalinter . --deadline  720s --vendor -D dupl -D gotype -D errcheck -D gas -D golint -E gofmt
+	gometalinter ./... --deadline  720s --vendor -D dupl -D gotype -D errcheck -D gas -D golint -D aligncheck -E gofmt
 
 format:
 	goimports -w -l $(APP_SOURCES)
diff --git a/apps/app.go b/apps/app.go
index c839809..5ea1fd1 100644
--- a/apps/app.go
+++ b/apps/app.go
@@ -105,12 +105,13 @@ func (app App) RegistrationIntentsNumber() int {
 func (app App) RegistrationIntents(task *Task, nameSeparator string) []RegistrationIntent {
 	taskPortsCount := len(task.Ports)
 	definitions := app.findConsulPortDefinitions()
+	commonTags := labelsToTags(app.Labels)
 	if len(definitions) == 0 && taskPortsCount != 0 {
 		return []RegistrationIntent{
 			{
 				Name: app.labelsToName(app.Labels, nameSeparator),
 				Port: task.Ports[0],
-				Tags: labelsToTags(app.Labels),
+				Tags: commonTags,
 			},
 		}
 	}
@@ -124,7 +125,7 @@ func (app App) RegistrationIntents(task *Task, nameSeparator string) []Registrat
 		intents = append(intents, RegistrationIntent{
 			Name: app.labelsToName(d.Labels, nameSeparator),
 			Port: task.Ports[d.Index],
-			Tags: append(labelsToTags(app.Labels), labelsToTags(d.Labels)...),
+			Tags: append(labelsToTags(d.Labels), commonTags...),
 		})
 	}
 	return intents
@@ -135,7 +136,7 @@ func marathonAppNameToServiceName(name string, nameSeparator string) string {
 }
 
 func labelsToTags(labels map[string]string) []string {
-	tags := []string{}
+	tags := make([]string, 0, len(labels))
 	for key, value := range labels {
 		if value == "tag" {
 			tags = append(tags, key)
diff --git a/apps/app.json b/apps/testdata/app.json
similarity index 100%
rename from apps/app.json
rename to apps/testdata/app.json
diff --git a/apps/apps.json b/apps/testdata/apps.json
similarity index 100%
rename from apps/apps.json
rename to apps/testdata/apps.json
diff --git a/apps/tasks.json b/apps/testdata/tasks.json
similarity index 100%
rename from apps/tasks.json
rename to apps/testdata/tasks.json
diff --git a/consul/consul.go b/consul/consul.go
index a368be3..c4b2410 100644
--- a/consul/consul.go
+++ b/consul/consul.go
@@ -173,7 +173,7 @@ func (c *Consul) registerMultipleServices(services []*consulapi.AgentServiceRegi
 		}
 	}
 
-	return utils.MergeErrorsOrNil(registerErrors, fmt.Sprint("registering services"))
+	return utils.MergeErrorsOrNil(registerErrors, "registering services")
 }
 
 func (c *Consul) register(service *consulapi.AgentServiceRegistration) error {
@@ -320,45 +320,53 @@ func (c *Consul) marathonToConsulChecks(task *apps.Task, healthChecks []apps.Hea
 			continue
 		}
 
-		consulCheck := consulapi.AgentServiceCheck{
-			Interval: fmt.Sprintf("%ds", check.IntervalSeconds),
-			Timeout:  fmt.Sprintf("%ds", check.TimeoutSeconds),
-			Status:   "passing",
+		if c := marathonToConsulCheck(task, check, serviceAddress, port); c != nil {
+			checks = append(checks, c)
 		}
 
-		switch check.Protocol {
-		case "HTTP", "HTTPS", "MESOS_HTTP", "MESOS_HTTPS":
-			path := "/"
-			if check.Path != "" {
-				path = check.Path
-			}
-			if parsedURL, err := url.ParseRequestURI(path); err == nil {
-				if check.Protocol == "HTTP" || check.Protocol == "MESOS_HTTP" {
-					parsedURL.Scheme = "http"
-				} else {
-					parsedURL.Scheme = "https"
-				}
-				parsedURL.Host = fmt.Sprintf("%s:%d", serviceAddress, port)
-				consulCheck.HTTP = parsedURL.String()
-				checks = append(checks, &consulCheck)
+	}
+	return checks
+}
+
+func marathonToConsulCheck(task *apps.Task, check apps.HealthCheck, serviceAddress string, port int) *consulapi.AgentServiceCheck {
+	consulCheck := &consulapi.AgentServiceCheck{
+		Interval: fmt.Sprintf("%ds", check.IntervalSeconds),
+		Timeout:  fmt.Sprintf("%ds", check.TimeoutSeconds),
+		Status:   "passing",
+	}
+
+	switch check.Protocol {
+	case "HTTP", "HTTPS", "MESOS_HTTP", "MESOS_HTTPS":
+		path := "/"
+		if check.Path != "" {
+			path = check.Path
+		}
+		if parsedURL, err := url.ParseRequestURI(path); err == nil {
+			if check.Protocol == "HTTP" || check.Protocol == "MESOS_HTTP" {
+				parsedURL.Scheme = "http"
 			} else {
-				log.WithError(err).
-					WithField("Id", task.AppID.String()).
-					WithField("Address", serviceAddress).
-					Warnf("Could not parse provided path: %s", path)
+				parsedURL.Scheme = "https"
 			}
-		case "TCP", "MESOS_TCP":
-			consulCheck.TCP = fmt.Sprintf("%s:%d", serviceAddress, port)
-			checks = append(checks, &consulCheck)
-		case "COMMAND":
-			consulCheck.Script = substituteEnvironment(check.Command.Value, *task)
-			checks = append(checks, &consulCheck)
-		default:
-			log.WithField("Id", task.AppID.String()).WithField("Address", serviceAddress).
-				Warnf("Unrecognized check protocol %s", check.Protocol)
+			parsedURL.Host = fmt.Sprintf("%s:%d", serviceAddress, port)
+			consulCheck.HTTP = parsedURL.String()
+			return consulCheck
+		} else {
+			log.WithError(err).
+				WithField("Id", task.AppID.String()).
+				WithField("Address", serviceAddress).
+				Warnf("Could not parse provided path: %s", path)
 		}
+	case "TCP", "MESOS_TCP":
+		consulCheck.TCP = fmt.Sprintf("%s:%d", serviceAddress, port)
+		return consulCheck
+	case "COMMAND":
+		consulCheck.Script = substituteEnvironment(check.Command.Value, *task)
+		return consulCheck
+	default:
+		log.WithField("Id", task.AppID.String()).WithField("Address", serviceAddress).
+			Warnf("Unrecognized check protocol %s", check.Protocol)
 	}
-	return checks
+	return nil
 }
 
 func getHealthCheckPort(check apps.HealthCheck, task apps.Task) (int, error) {
diff --git a/events/sse_events.go b/events/sse_events.go
index eba2589..04e6c65 100644
--- a/events/sse_events.go
+++ b/events/sse_events.go
@@ -9,11 +9,10 @@ import (
 
 // Event holds state of parsed fields from marathon EventStream
 type SSEEvent struct {
-	Type        string
-	Body        []byte
-	ID          string
-	Delay       string
-	maxLineSize int64
+	Type  string
+	Body  []byte
+	ID    string
+	Delay string
 }
 
 var (
@@ -29,7 +28,7 @@ func (e *SSEEvent) parseLine(line []byte) bool {
 	// or a single U+000D CARRIAGE RETURN (CR) character.
 
 	//If the line is empty (a blank line)
-	if len(line) == 0 || bytes.Compare(line, lineFeed) == 0 {
+	if len(line) == 0 || bytes.Equal(line, lineFeed) {
 		//Dispatch the event, as defined below.
 		return !e.isEmpty()
 	}
diff --git a/sse/sse.go b/sse/sse.go
index 5af07fc..bec5e33 100644
--- a/sse/sse.go
+++ b/sse/sse.go
@@ -16,7 +16,7 @@ type Stop func()
 type Handler func(w http.ResponseWriter, r *http.Request)
 
 func NewHandler(config Config, webConfig web.Config, marathon marathon.Marathoner, serviceOperations service.ServiceRegistry) (Stop, error) {
-	stopChannels := make([]chan<- events.StopEvent, webConfig.WorkersCount, webConfig.WorkersCount)
+	stopChannels := make([]chan<- events.StopEvent, webConfig.WorkersCount)
 	stopFunc := stop(stopChannels)
 	eventQueue := make(chan events.Event, webConfig.QueueSize)
 	for i := 0; i < webConfig.WorkersCount; i++ {
@@ -38,7 +38,7 @@ func NewHandler(config Config, webConfig web.Config, marathon marathon.Marathone
 	guardQuit := leaderGuard(sse.Streamer, marathon)
 	stopChannels = append(stopChannels, dispatcherStop, guardQuit)
 
-	return stopFunc, nil
+	return stop(stopChannels), nil
 }
 
 func stop(channels []chan<- events.StopEvent) Stop {
@@ -59,12 +59,13 @@ func leaderGuard(s *marathon.Streamer, m marathon.Marathoner) chan<- events.Stop
 	quit := make(chan events.StopEvent)
 
 	go func() {
-		ticker := time.Tick(5 * time.Second)
+		ticker := time.NewTicker(5 * time.Second)
 		for {
 			select {
-			case <-ticker:
+			case <-ticker.C:
 				if iAMLeader, err := m.IsLeader(); !iAMLeader && err != nil {
 					// Leader changed, not revocerable.
+					ticker.Stop()
 					s.Stop()
 					log.Error("Tearing down SSE stream, marathon leader changed.")
 					return
@@ -73,6 +74,7 @@ func leaderGuard(s *marathon.Streamer, m marathon.Marathoner) chan<- events.Stop
 				}
 			case <-quit:
 				log.Info("Recieved quit notification. Quit checker")
+				ticker.Stop()
 				s.Stop()
 				return
 			}
diff --git a/sse/sse_handler.go b/sse/sse_handler.go
index 490aafb..a0eaf5a 100644
--- a/sse/sse_handler.go
+++ b/sse/sse_handler.go
@@ -1,10 +1,8 @@
 package sse
 
 import (
-	"context"
 	"fmt"
 	"io"
-	"net/http"
 	"time"
 
 	log "github.com/Sirupsen/logrus"
@@ -18,10 +16,6 @@ import (
 type SSEHandler struct {
 	config      Config
 	eventQueue  chan events.Event
-	loc         string
-	client      *http.Client
-	close       context.CancelFunc
-	req         *http.Request
 	Streamer    *marathon.Streamer
 	maxLineSize int64
 }
diff --git a/sync/sync.go b/sync/sync.go
index b773433..8122fc4 100644
--- a/sync/sync.go
+++ b/sync/sync.go
@@ -47,7 +47,6 @@ func (s *Sync) StartSyncServicesJob() {
 			}
 		}
 	}()
-	return
 }
 
 func (s *Sync) SyncServices() error {
diff --git a/time/time.go b/time/time.go
index c81fede..04d73be 100644
--- a/time/time.go
+++ b/time/time.go
@@ -26,7 +26,7 @@ func (t Timestamp) MarshalJSON() ([]byte, error) {
 }
 
 func (t *Timestamp) Delay() time.Duration {
-	return time.Now().Sub(t.Time)
+	return time.Since(t.Time)
 }
 
 func (t *Timestamp) String() string {
diff --git a/web/web.go b/web/web.go
index d305673..9d9908c 100644
--- a/web/web.go
+++ b/web/web.go
@@ -13,7 +13,7 @@ type Handler func(w http.ResponseWriter, r *http.Request)
 
 func NewHandler(config Config, marathon marathon.Marathoner, serviceOperations service.ServiceRegistry) (Handler, Stop) {
 
-	stopChannels := make([]chan<- events.StopEvent, config.WorkersCount, config.WorkersCount)
+	stopChannels := make([]chan<- events.StopEvent, config.WorkersCount)
 	eventQueue := make(chan events.Event, config.QueueSize)
 	for i := 0; i < config.WorkersCount; i++ {
 		handler := events.NewEventHandler(i, serviceOperations, marathon, eventQueue)


Test patch:
diff --git a/apps/app_bench_test.go b/apps/app_bench_test.go
new file mode 100644
index 0000000..79f4505
--- /dev/null
+++ b/apps/app_bench_test.go
@@ -0,0 +1,33 @@
+package apps
+
+import "testing"
+
+func BenchmarkApp_RegistrationIntents(b *testing.B) {
+
+	// given an average app.
+	// Over 80% of our apps has only one port
+	// registered and more then five (5) labels but only 2 of them
+	// are tags. Over 90% of our application does not have
+	// special tags on ports.
+	app := &App{
+		ID: "app-name",
+		Labels: map[string]string{
+			"#1":     "label",
+			"#2":     "label",
+			"consul": "true",
+			"#3":     "label",
+			"#4":     "tag",
+			"#5":     "label",
+			"#6":     "tag",
+			"#7":     "label",
+		},
+		PortDefinitions: []PortDefinition{{Labels: map[string]string{"#8": "tag"}}},
+	}
+	task := &Task{
+		Ports: []int{0},
+	}
+
+	for i := 0; i < b.N; i++ {
+		app.RegistrationIntents(task, "-")
+	}
+}
diff --git a/apps/app_test.go b/apps/app_test.go
index 13dd754..b13a66b 100644
--- a/apps/app_test.go
+++ b/apps/app_test.go
@@ -10,7 +10,7 @@ import (
 func TestParseApps(t *testing.T) {
 	t.Parallel()
 
-	appBlob, _ := ioutil.ReadFile("apps.json")
+	appBlob, _ := ioutil.ReadFile("testdata/apps.json")
 
 	expected := []*App{
 		{
@@ -58,18 +58,18 @@ func TestAppInt(t *testing.T) {
 	assert.Equal(t, 2, app.RegistrationIntentsNumber())
 
 	task := Task{Ports: []int{0, 1, 2, 3}}
-	intents := app.RegistrationIntents(&task,".")
+	intents := app.RegistrationIntents(&task, ".")
 
-	assert.Contains(t,  intents[0].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
-	assert.Contains(t,  intents[1].Tags, "secureConnection:true")
-	assert.NotContains(t,  intents[0].Tags, "secureConnection:true")
-	assert.NotContains(t,  intents[1].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
+	assert.Contains(t, intents[0].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
+	assert.Contains(t, intents[1].Tags, "secureConnection:true")
+	assert.NotContains(t, intents[0].Tags, "secureConnection:true")
+	assert.NotContains(t, intents[1].Tags, "Lorem ipsum dolor sit amet, consectetur adipiscing elit")
 }
 
 func TestParseApp(t *testing.T) {
 	t.Parallel()
 
-	appBlob, _ := ioutil.ReadFile("app.json")
+	appBlob, _ := ioutil.ReadFile("testdata/app.json")
 
 	expected := &App{Labels: map[string]string{"consul": "true", "public": "tag"},
 		HealthChecks: []HealthCheck{
@@ -312,7 +312,7 @@ func TestRegistrationIntent_OverrideNameAndAddTagsViaPortDefinitions(t *testing.
 	assert.Len(t, intents, 1)
 	assert.Equal(t, "other-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"private", "other"}, intents[0].Tags)
+	assert.Equal(t, []string{"other", "private"}, intents[0].Tags)
 }
 
 func TestRegistrationIntent_PickDifferentPortViaPortDefinitions(t *testing.T) {
@@ -367,10 +367,10 @@ func TestRegistrationIntent_MultipleIntentsViaPortDefinitionIfMultipleContainCon
 	assert.Len(t, intents, 2)
 	assert.Equal(t, "first-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"common-tag", "first-tag"}, intents[0].Tags)
+	assert.Equal(t, []string{"first-tag", "common-tag"}, intents[0].Tags)
 	assert.Equal(t, "second-name", intents[1].Name)
 	assert.Equal(t, 5678, intents[1].Port)
-	assert.Equal(t, []string{"common-tag", "second-tag"}, intents[1].Tags)
+	assert.Equal(t, []string{"second-tag", "common-tag"}, intents[1].Tags)
 }
 
 func TestRegistrationIntent_TaskHasLessPortsThanApp(t *testing.T) {
@@ -400,7 +400,7 @@ func TestRegistrationIntent_TaskHasLessPortsThanApp(t *testing.T) {
 	assert.Len(t, intents, 1)
 	assert.Equal(t, "first-name", intents[0].Name)
 	assert.Equal(t, 1234, intents[0].Port)
-	assert.Equal(t, []string{"common-tag", "first-tag"}, intents[0].Tags)
+	assert.Equal(t, []string{"first-tag", "common-tag"}, intents[0].Tags)
 }
 
 func TestRegistrationIntentsNumber(t *testing.T) {
diff --git a/apps/task_test.go b/apps/task_test.go
index 0bba314..a1f1a0b 100644
--- a/apps/task_test.go
+++ b/apps/task_test.go
@@ -37,7 +37,7 @@ func TestParseTask(t *testing.T) {
 func TestParseTasks(t *testing.T) {
 	t.Parallel()
 
-	tasksBlob, _ := ioutil.ReadFile("tasks.json")
+	tasksBlob, _ := ioutil.ReadFile("testdata/tasks.json")
 
 	expectedTasks := []Task{
 		{
@@ -118,7 +118,10 @@ func TestId_AppId(t *testing.T) {
 	assert.Equal(t, AppID("/pl.allegro/test/app"), TaskID(id).AppID())
 }
 
-func TestId_AppIdForInvalid(t *testing.T) {
+func TestId_AppIdForInvalidIdShouldPanic(t *testing.T) {
 	t.Parallel()
-	assert.Panics(t, func() { TaskID("id").AppID() })
+	assert.Panics(t, func() {
+		a := TaskID("id").AppID()
+		assert.Nil(t, a)
+	})
 }
diff --git a/consul/consul_test.go b/consul/consul_test.go
index 9393872..a78aa38 100644
--- a/consul/consul_test.go
+++ b/consul/consul_test.go
@@ -452,9 +452,9 @@ func TestRegisterServices_MultipleRegistrations(t *testing.T) {
 	assert.True(t, found, "second-name not found in services")
 
 	assert.Equal(t, "first-name", first.Name)
-	assert.Equal(t, []string{"marathon", "common-tag", "first-tag", "marathon-task:serviceA.0"}, first.Tags)
+	assert.Equal(t, []string{"marathon", "first-tag", "common-tag", "marathon-task:serviceA.0"}, first.Tags)
 	assert.Equal(t, "second-name", second.Name)
-	assert.Equal(t, []string{"marathon", "common-tag", "second-tag", "marathon-task:serviceA.0"}, second.Tags)
+	assert.Equal(t, []string{"marathon", "second-tag", "common-tag", "marathon-task:serviceA.0"}, second.Tags)
 }
 
 func findServiceByName(name string, services []*service.Service) (*service.Service, bool) {
diff --git a/events/event_handler_test.go b/events/event_handler_test.go
index f4b17fc..32ac587 100644
--- a/events/event_handler_test.go
+++ b/events/event_handler_test.go
@@ -1,7 +1,6 @@
 package events
 
 import (
-	"errors"
 	"strconv"
 	"testing"
 	"time"
@@ -405,12 +404,6 @@ func TestEventHandler_HandleHealthStatusEventWhenTaskIsNotInMarathon(t *testing.
 	assert.True(t, marathon.Interactions())
 }
 
-type BadReader struct{}
-
-func (r BadReader) Read(p []byte) (int, error) {
-	return 0, errors.New("Some error")
-}
-
 func healthStatusChangeEventForTask(taskID string) []byte {
 	return []byte(`{
 	  "appId":"/test/app",
diff --git a/events/sse_events_test.go b/events/sse_events_test.go
index 3cbdf3a..f226009 100644
--- a/events/sse_events_test.go
+++ b/events/sse_events_test.go
@@ -137,37 +137,37 @@ var parseEventMultipleDataCases = []struct {
 }{
 	{"\n\n\n\n\n\n\n",
 		[]SSEEvent{
-			SSEEvent{},
+			{},
 		},
 	},
 	{"event: status_update_event\ndata: testData\n\nevent: some_event\ndata: someData",
 		[]SSEEvent{
-			SSEEvent{Type: "status_update_event", Body: []byte("testData\n")},
-			SSEEvent{Type: "some_event", Body: []byte("someData\n")},
+			{Type: "status_update_event", Body: []byte("testData\n")},
+			{Type: "some_event", Body: []byte("someData\n")},
 		},
 	},
 	{"event: status_update_event\ndata: testData\n\nid: 13\ndata: someData\n\nid: 14\ndata: abc\n\nid: 15\ndata: def\n",
 		[]SSEEvent{
-			SSEEvent{Type: "status_update_event", Body: []byte("testData\n")},
-			SSEEvent{ID: "13", Body: []byte("someData\n")},
-			SSEEvent{ID: "14", Body: []byte("abc\n")},
-			SSEEvent{ID: "15", Body: []byte("def\n")},
+			{Type: "status_update_event", Body: []byte("testData\n")},
+			{ID: "13", Body: []byte("someData\n")},
+			{ID: "14", Body: []byte("abc\n")},
+			{ID: "15", Body: []byte("def\n")},
 		},
 	},
 	{"data: testData\n\ndata: someData\n\ndata: abc\n\ndata: def\n",
 		[]SSEEvent{
-			SSEEvent{Body: []byte("testData\n")},
-			SSEEvent{Body: []byte("someData\n")},
-			SSEEvent{Body: []byte("abc\n")},
-			SSEEvent{Body: []byte("def\n")},
+			{Body: []byte("testData\n")},
+			{Body: []byte("someData\n")},
+			{Body: []byte("abc\n")},
+			{Body: []byte("def\n")},
 		},
 	},
 	{"data: testData\nretry: 10\ndummy: dummy field\n\ndata: someData\n\ndata: abc\n\ndata: def\n",
 		[]SSEEvent{
-			SSEEvent{Body: []byte("testData\n"), Delay: "10"},
-			SSEEvent{Body: []byte("someData\n")},
-			SSEEvent{Body: []byte("abc\n")},
-			SSEEvent{Body: []byte("def\n")},
+			{Body: []byte("testData\n"), Delay: "10"},
+			{Body: []byte("someData\n")},
+			{Body: []byte("abc\n")},
+			{Body: []byte("def\n")},
 		},
 	},
 }
diff --git a/sync/sync_test.go b/sync/sync_test.go
index 445c49b..843de20 100644
--- a/sync/sync_test.go
+++ b/sync/sync_test.go
@@ -383,7 +383,7 @@ func TestSync_WithDeregisteringFallback(t *testing.T) {
 		consulStub.Register(&task, marathonApp)
 	}
 	marathon.TasksStub = map[apps.AppID][]apps.Task{
-		apps.AppID("/test/app"): []apps.Task{marathonApp.Tasks[0]},
+		apps.AppID("/test/app"): {marathonApp.Tasks[0]},
 	}
 	sync := newSyncWithDefaultConfig(marathon, consulStub)