ref(storage,syslogish): make all storage adapter initialization funcs…

… return the storage.Adapter interface (#93)

Fixes #89

Also:

- Flatten all storage adapters and the storage factory into a single storage package
- Move the storage adapter factory into a factory subpackage of 'github.com/deis/logger/storage'
  - Eliminates a circular dependency from the storage adapter packages and storage
- Add a 'testredis' build tag to the './storage/redis_adapter_test.go' (which was added in #88), so that a developer can still run 'go test $(glide nv)' without requiring a redis server be up and running
  - 'make test', however, still runs tests with that tag enabled, because it also runs a redis server in a linked container

.gitignore

@@ -2,3 +2,4 @@
 /manifests/*.tmp.yaml
 vendor/
 coverage.txt
+logger

Makefile

@@ -96,7 +96,11 @@ style-check:
 	$(GOFMT) $(GO_PACKAGES) $(GO_FILES)
 	@$(GOFMT) $(GO_PACKAGES) $(GO_FILES) | read; if [ $$? == 0 ]; then echo "gofmt check failed."; exit 1; fi
 	$(GOVET) $(REPO_PATH) $(GO_PACKAGES_REPO_PATH)
-	$(GOLINT) ./...
+	$(GOLINT) ./log
+	$(GOLINT) ./storage
+	$(GOLINT) ./tests
+	$(GOLINT) ./weblog
+	$(GOLINT) .
 	shellcheck $(SHELL_SCRIPTS)
 
 start-test-redis:
@@ -112,7 +116,7 @@ test-unit: start-test-redis
 		--link ${REDIS_CONTAINER_NAME}:TEST_REDIS \
 		${DEV_ENV_IMAGE} bash -c 'DEIS_LOGGER_REDIS_SERVICE_HOST=$$TEST_REDIS_PORT_6379_TCP_ADDR \
 		DEIS_LOGGER_REDIS_SERVICE_PORT=$$TEST_REDIS_PORT_6379_TCP_PORT \
-		$(GOTEST) $$(glide nv)' \
+		$(GOTEST) -tags="testredis" $$(glide nv)' \
 		|| (make stop-test-redis && false)
 	make stop-test-redis
 

log/model.go

@@ -17,7 +17,7 @@ type Message struct {
 	Docker     Docker     `json:"docker"`
 }
 
-// Kuberentes specific log message fields
+// Kubernetes specific log message fields
 type Kubernetes struct {
 	Namespace     string            `json:"namespace_name"`
 	PodID         string            `json:"pod_id"`

storage/factory.go

@@ -2,34 +2,39 @@ package storage
 
 import (
 	"fmt"
-	"github.com/deis/logger/storage/file"
-	"github.com/deis/logger/storage/redis"
-	"github.com/deis/logger/storage/ringbuffer"
 )
 
+type errUnrecognizedStorageAdapterType struct {
+	adapterType string
+}
+
+func (e errUnrecognizedStorageAdapterType) Error() string {
+	return fmt.Sprintf("Unrecognized storage adapter type: %s", e.adapterType)
+}
+
 // NewAdapter returns a pointer to an appropriate implementation of the Adapter interface, as
-// determined by the storeageAdapterType string it is passed.
-func NewAdapter(storeageAdapterType string, numLines int) (Adapter, error) {
-	if storeageAdapterType == "file" {
-		adapter, err := file.NewStorageAdapter()
+// determined by the adapterType string it is passed.
+func NewAdapter(adapterType string, numLines int) (Adapter, error) {
+	if adapterType == "file" {
+		adapter, err := NewFileAdapter()
 		if err != nil {
 			return nil, err
 		}
 		return adapter, nil
 	}
-	if storeageAdapterType == "memory" {
-		adapter, err := ringbuffer.NewStorageAdapter(numLines)
+	if adapterType == "memory" {
+		adapter, err := NewRingBufferAdapter(numLines)
 		if err != nil {
 			return nil, err
 		}
 		return adapter, nil
 	}
-	if storeageAdapterType == "redis" {
-		adapter, err := redis.NewStorageAdapter(numLines)
+	if adapterType == "redis" {
+		adapter, err := NewRedisStorageAdapter(numLines)
 		if err != nil {
 			return nil, err
 		}
 		return adapter, nil
 	}
-	return nil, fmt.Errorf("Unrecognized storage adapter type: '%s'", storeageAdapterType)
+	return nil, errUnrecognizedStorageAdapterType{adapterType: adapterType}
 }

storage/factory_test.go

@@ -1,40 +1,48 @@
 package storage
 
 import (
-	"fmt"
-	"os"
 	"reflect"
 	"testing"
 )
 
-func TestGetUsingInvalidValues(t *testing.T) {
-	_, err := NewAdapter("bogus", 1)
-	if err == nil || err.Error() != fmt.Sprintf("Unrecognized storage adapter type: '%s'", "bogus") {
-		t.Error("Did not receive expected error message")
+const (
+	app = "test-app"
+)
+
+func TestFactoryGetUsingInvalidValues(t *testing.T) {
+	const adapterType = "bogus"
+	_, err := NewAdapter(adapterType, 1)
+	if err == nil {
+		t.Fatalf("Did not receive an error message")
+	}
+	unrecognizedErr, ok := err.(errUnrecognizedStorageAdapterType)
+	if !ok {
+		t.Fatalf("Expected an errUnrecognizedStorageAdapterType, received %s", err)
+	}
+	if unrecognizedErr.adapterType != adapterType {
+		t.Fatalf("Got an errUnrecognizedStorageAdapterType, but expected adapter type %s, got %s", adapterType, unrecognizedErr.adapterType)
 	}
 }
 
-func TestGetFileBasedAdapter(t *testing.T) {
+func TestFactoryGetFileBasedAdapter(t *testing.T) {
 	a, err := NewAdapter("file", 1)
 	if err != nil {
 		t.Error(err)
 	}
-	expected := "*file.adapter"
-	aType := reflect.TypeOf(a).String()
-	if aType != expected {
-		t.Errorf("Expected a %s, but got a %s", expected, aType)
+	retType, ok := a.(*fileAdapter)
+	if !ok {
+		t.Fatalf("Expected a *fileAdapter, got %s", reflect.TypeOf(retType).String())
 	}
 }
 
-func TestGetMemoryBasedAdapter(t *testing.T) {
+func TestFactoryGetMemoryBasedAdapter(t *testing.T) {
 	a, err := NewAdapter("memory", 1)
 	if err != nil {
 		t.Error(err)
 	}
-	expected := "*ringbuffer.adapter"
-	aType := reflect.TypeOf(a).String()
-	if aType != expected {
-		t.Errorf("Expected a %s, but got a %s", expected, aType)
+	retType, ok := a.(*ringBufferAdapter)
+	if !ok {
+		t.Fatalf("Expected a *ringBufferAdapter, got %s", reflect.TypeOf(retType).String())
 	}
 }
 
@@ -43,13 +51,8 @@ func TestGetRedisBasedAdapter(t *testing.T) {
 	if err != nil {
 		t.Error(err)
 	}
-	expected := "*redis.adapter"
-	aType := reflect.TypeOf(a).String()
-	if aType != expected {
-		t.Errorf("Expected a %s, but got a %s", expected, aType)
+	retType, ok := a.(*redisAdapter)
+	if !ok {
+		t.Errorf("Expected a redisAdapter, but got a %s", reflect.TypeOf(retType).String())
 	}
 }
-
-func TestMain(m *testing.M) {
-	os.Exit(m.Run())
-}

storage/file/adapter.go → storage/file_adapter.go

@@ -1,4 +1,4 @@
-package file
+package storage
 
 import (
 	"fmt"
@@ -12,18 +12,18 @@ import (
 
 var logRoot = "/data/logs"
 
-type adapter struct {
+type fileAdapter struct {
 	files map[string]*os.File
 	mutex sync.Mutex
 }
 
-// NewStorageAdapter returns a pointer to a new instance of a file-based storage.Adapter.
-func NewStorageAdapter() (*adapter, error) {
-	return &adapter{files: make(map[string]*os.File)}, nil
+// NewFileAdapter returns an Adapter that uses a file.
+func NewFileAdapter() (Adapter, error) {
+	return &fileAdapter{files: make(map[string]*os.File)}, nil
 }
 
 // Write adds a log message to to an app-specific log file
-func (a *adapter) Write(app string, message string) error {
+func (a *fileAdapter) Write(app string, message string) error {
 	// Check first if we might actually have to add to the map of file pointers so we can avoid
 	// waiting for / obtaining a lock unnecessarily
 	f, ok := a.files[app]
@@ -49,7 +49,7 @@ func (a *adapter) Write(app string, message string) error {
 }
 
 // Read retrieves a specified number of log lines from an app-specific log file
-func (a *adapter) Read(app string, lines int) ([]string, error) {
+func (a *fileAdapter) Read(app string, lines int) ([]string, error) {
 	if lines <= 0 {
 		return []string{}, nil
 	}
@@ -70,7 +70,7 @@ func (a *adapter) Read(app string, lines int) ([]string, error) {
 }
 
 // Destroy deletes stored logs for the specified application
-func (a *adapter) Destroy(app string) error {
+func (a *fileAdapter) Destroy(app string) error {
 	// Check first if the map of file pointers even contains the file pointer we want so we can avoid
 	// waiting for / obtaining a lock unnecessarily
 	f, ok := a.files[app]
@@ -93,7 +93,7 @@ func (a *adapter) Destroy(app string) error {
 	return nil
 }
 
-func (a *adapter) Reopen() error {
+func (a *fileAdapter) Reopen() error {
 	// Ensure no other goroutine is trying to add a file pointer to the map of file pointers while
 	// we're trying to clear it out
 	a.mutex.Lock()
@@ -102,7 +102,7 @@ func (a *adapter) Reopen() error {
 	return nil
 }
 
-func (a *adapter) getFile(app string) (*os.File, error) {
+func (a *fileAdapter) getFile(app string) (*os.File, error) {
 	filePath := a.getFilePath(app)
 	exists, err := fileExists(filePath)
 	if err != nil {
@@ -118,7 +118,7 @@ func (a *adapter) getFile(app string) (*os.File, error) {
 	return file, err
 }
 
-func (a *adapter) getFilePath(app string) string {
+func (a *fileAdapter) getFilePath(app string) string {
 	return path.Join(logRoot, app+".log")
 }
 

storage/file/adapter_test.go → storage/file_adapter_test.go

@@ -1,4 +1,4 @@
-package file
+package storage
 
 import (
 	"fmt"
@@ -8,8 +8,6 @@ import (
 	"testing"
 )
 
-const app string = "test-app"
-
 func TestReadFromNonExistingApp(t *testing.T) {
 	var err error
 	logRoot, err = ioutil.TempDir("", "log-tests")
@@ -18,7 +16,7 @@ func TestReadFromNonExistingApp(t *testing.T) {
 	}
 	defer os.Remove(logRoot)
 	// Initialize a new storage adapter
-	a, err := NewStorageAdapter()
+	a, err := NewFileAdapter()
 	if err != nil {
 		t.Error(err)
 	}
@@ -39,7 +37,7 @@ func TestLogs(t *testing.T) {
 		t.Error(err)
 	}
 	defer os.Remove(logRoot)
-	a, err := NewStorageAdapter()
+	a, err := NewFileAdapter()
 	if err != nil {
 		t.Error(err)
 	}
@@ -65,7 +63,7 @@ func TestLogs(t *testing.T) {
 	}
 	// Should get the 3 MOST RECENT logs
 	if len(messages) != 3 {
-		t.Error("only expected 5 log messages, got %d", len(messages))
+		t.Errorf("only expected 5 log messages, got %d", len(messages))
 	}
 	for i := 0; i < 3; i++ {
 		expectedMessage := fmt.Sprintf("message %d", i+2)
@@ -82,10 +80,16 @@ func TestDestroy(t *testing.T) {
 		t.Error(err)
 	}
 	defer os.Remove(logRoot)
-	a, err := NewStorageAdapter()
+	sa, err := NewFileAdapter()
 	if err != nil {
 		t.Error(err)
 	}
+
+	a, ok := sa.(*fileAdapter)
+	if !ok {
+		t.Fatalf("returned adapter was not a ringBuffer")
+	}
+
 	// Write a log to create the file
 	if err := a.Write(app, "Hello, log!"); err != nil {
 		t.Error(err)
@@ -116,10 +120,14 @@ func TestReopen(t *testing.T) {
 		t.Error(err)
 	}
 	defer os.Remove(logRoot)
-	a, err := NewStorageAdapter()
+	sa, err := NewFileAdapter()
 	if err != nil {
 		t.Error(err)
 	}
+	a, ok := sa.(*fileAdapter)
+	if !ok {
+		t.Fatalf("returned adapter was not a ringBuffer")
+	}
 	// Write a log to create the file
 	if err := a.Write(app, "Hello, log!"); err != nil {
 		t.Error(err)

storage/redis/adapter.go → storage/redis_adapter.go

@@ -1,4 +1,4 @@
-package redis
+package storage
 
 import (
 	"fmt"
@@ -7,13 +7,13 @@ import (
 	r "gopkg.in/redis.v3"
 )
 
-type adapter struct {
+type redisAdapter struct {
 	bufferSize  int
 	redisClient *r.Client
 }
 
-// NewStorageAdapter returns a pointer to a new instance of a redis-based storage.Adapter.
-func NewStorageAdapter(bufferSize int) (*adapter, error) {
+// NewRedisStorageAdapter returns a pointer to a new instance of a redis-based storage.Adapter.
+func NewRedisStorageAdapter(bufferSize int) (*redisAdapter, error) {
 	if bufferSize <= 0 {
 		return nil, fmt.Errorf("Invalid buffer size: %d", bufferSize)
 	}
@@ -24,7 +24,7 @@ func NewStorageAdapter(bufferSize int) (*adapter, error) {
 	if err != nil {
 		return nil, err
 	}
-	return &adapter{
+	return &redisAdapter{
 		bufferSize: bufferSize,
 		redisClient: r.NewClient(&r.Options{
 			Addr:     fmt.Sprintf("%s:%d", cfg.RedisHost, cfg.RedisPort),
@@ -35,7 +35,7 @@ func NewStorageAdapter(bufferSize int) (*adapter, error) {
 }
 
 // Write adds a log message to to an app-specific list in redis using ring-buffer-like semantics
-func (a *adapter) Write(app string, message string) error {
+func (a *redisAdapter) Write(app string, message string) error {
 	// Note: Deliberately NOT using MULTI / transactions here since in this implementation of the
 	// redis client, MULTI is not safe for concurrent use by multiple goroutines. It's been advised
 	// by the authors of the gopkg.in/redis.v3 package to just use pipelining when possible...
@@ -57,7 +57,7 @@ func (a *adapter) Write(app string, message string) error {
 }
 
 // Read retrieves a specified number of log lines from an app-specific list in redis
-func (a *adapter) Read(app string, lines int) ([]string, error) {
+func (a *redisAdapter) Read(app string, lines int) ([]string, error) {
 	stringSliceCmd := a.redisClient.LRange(app, int64(-1*lines), -1)
 	result, err := stringSliceCmd.Result()
 	if err != nil {
@@ -70,14 +70,14 @@ func (a *adapter) Read(app string, lines int) ([]string, error) {
 }
 
 // Destroy deletes an app-specific list from redis
-func (a *adapter) Destroy(app string) error {
+func (a *redisAdapter) Destroy(app string) error {
 	if err := a.redisClient.Del(app).Err(); err != nil {
 		return err
 	}
 	return nil
 }
 
-func (a *adapter) Reopen() error {
+func (a *redisAdapter) Reopen() error {
 	// No-op
 	return nil
 }