refactor code

* static registry should have only the template but not a path * file registry should ready the template * move logging into main.go * simplify tests * add command line arg tests
fabiolb · Dec 16, 2017 · 1feb028 · 1feb028
1 parent b84bb2a
commit 1feb028
Show file tree

Hide file tree

Showing 8 changed files with 61 additions and 51 deletions.
diff --git a/config/config.go b/config/config.go
@@ -108,13 +108,13 @@ type Registry struct {
 }
 
 type Static struct {
-	NoRouteHTMLPath string
-	Routes          string
+	NoRouteHTML string
+	Routes      string
 }
 
 type File struct {
 	NoRouteHTMLPath string
-	Path            string
+	RoutesPath      string
 }
 
 type Consul struct {

diff --git a/config/load.go b/config/load.go
@@ -155,10 +155,10 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
 	f.StringVar(&cfg.Registry.Backend, "registry.backend", defaultConfig.Registry.Backend, "registry backend")
 	f.DurationVar(&cfg.Registry.Timeout, "registry.timeout", defaultConfig.Registry.Timeout, "timeout for registry to become available")
 	f.DurationVar(&cfg.Registry.Retry, "registry.retry", defaultConfig.Registry.Retry, "retry interval during startup")
-	f.StringVar(&cfg.Registry.File.Path, "registry.file.path", defaultConfig.Registry.File.Path, "path to file based routing table")
+	f.StringVar(&cfg.Registry.File.RoutesPath, "registry.file.path", defaultConfig.Registry.File.RoutesPath, "path to file based routing table")
 	f.StringVar(&cfg.Registry.File.NoRouteHTMLPath, "registry.file.noroutehtmlpath", defaultConfig.Registry.File.NoRouteHTMLPath, "path to file for HTML returned when no route is found")
 	f.StringVar(&cfg.Registry.Static.Routes, "registry.static.routes", defaultConfig.Registry.Static.Routes, "static routes")
-	f.StringVar(&cfg.Registry.Static.NoRouteHTMLPath, "registry.static.noroutehtmlpath", defaultConfig.Registry.Static.NoRouteHTMLPath, "path to file for HTML returned when no route is found")
+	f.StringVar(&cfg.Registry.Static.NoRouteHTML, "registry.static.noroutehtml", defaultConfig.Registry.Static.NoRouteHTML, "HTML which is returned when no route is found")
 	f.StringVar(&cfg.Registry.Consul.Addr, "registry.consul.addr", defaultConfig.Registry.Consul.Addr, "address of the consul agent")
 	f.StringVar(&cfg.Registry.Consul.Token, "registry.consul.token", defaultConfig.Registry.Consul.Token, "token for consul agent")
 	f.StringVar(&cfg.Registry.Consul.KVPath, "registry.consul.kvpath", defaultConfig.Registry.Consul.KVPath, "consul KV path for manual overrides")

diff --git a/config/load_test.go b/config/load_test.go
@@ -428,7 +428,14 @@ func TestLoad(t *testing.T) {
 		{
 			args: []string{"-registry.file.path", "value"},
 			cfg: func(cfg *Config) *Config {
-				cfg.Registry.File.Path = "value"
+				cfg.Registry.File.RoutesPath = "value"
+				return cfg
+			},
+		},
+		{
+			args: []string{"-registry.file.noroutehtmlpath", "value"},
+			cfg: func(cfg *Config) *Config {
+				cfg.Registry.File.NoRouteHTMLPath = "value"
 				return cfg
 			},
 		},
@@ -439,6 +446,13 @@ func TestLoad(t *testing.T) {
 				return cfg
 			},
 		},
+		{
+			args: []string{"-registry.static.noroutehtml", "value"},
+			cfg: func(cfg *Config) *Config {
+				cfg.Registry.Static.NoRouteHTML = "value"
+				return cfg
+			},
+		},
 		{
 			args: []string{"-registry.consul.addr", "1.2.3.4:5555"},
 			cfg: func(cfg *Config) *Config {
@@ -485,6 +499,13 @@ func TestLoad(t *testing.T) {
 				return cfg
 			},
 		},
+		{
+			args: []string{"-registry.consul.noroutehtmlpath", "/some/path"},
+			cfg: func(cfg *Config) *Config {
+				cfg.Registry.Consul.NoRouteHTMLPath = "/some/path"
+				return cfg
+			},
+		},
 		{
 			args: []string{"-registry.consul.tagprefix", "p-"},
 			cfg: func(cfg *Config) *Config {

diff --git a/main.go b/main.go
@@ -410,17 +410,18 @@ func watchBackend(cfg *config.Config, first chan bool) {
 }
 
 func watchNoRouteHTML(cfg *config.Config) {
-	var last string
 	html := registry.Default.WatchNoRouteHTML()
-
 	for {
 		next := <-html
-
-		if next == last {
+		if next == noroute.GetHTML() {
 			continue
 		}
 		noroute.SetHTML(next)
-		last = next
+		if next == "" {
+			log.Print("[INFO] Unset noroute HTML")
+		} else {
+			log.Printf("[INFO] Set noroute HTML (%d bytes)", len(next))
+		}
 	}
 }
 

diff --git a/noroute/store.go b/noroute/store.go
@@ -1,29 +1,21 @@
 package noroute
 
 import (
-    "log"
-    "sync/atomic"
+	"sync/atomic"
 )
 
 var store atomic.Value // string
 
 func init() {
-    store.Store("")
+	store.Store("")
 }
 
 // GetHTML returns the HTML for not found routes.
 func GetHTML() string {
-    return store.Load().(string)
+	return store.Load().(string)
 }
 
-// SetHTML sets the current noroute html.
+// SetHTML sets the HTML for not found routes.
 func SetHTML(h string) {
-    // html := HTML{h}
-    store.Store(h)
-
-    if h == "" {
-        log.Print("[INFO] Unset noroute HTML")
-    } else {
-        log.Printf("[INFO] Set noroute HTML (%d bytes)", len(h))
-    }
+	store.Store(h)
 }
diff --git a/noroute/store_test.go b/noroute/store_test.go
@@ -1,19 +1,16 @@
 package noroute
 
 import (
-    "testing"
+	"testing"
 )
 
 func TestStoreSetGet(t *testing.T) {
-    got := GetHTML()
-    if got != "" {
-        t.Fatalf("Expected unset noroute html to be an empty string, got %s", got)
-    }
+	if got, want := GetHTML(), ""; got != want {
+		t.Fatalf("got unset noroute html %q want %q", got, want)
+	}
 
-    want := "<blink>Fancy!</blink>"
-    SetHTML(want)
-    got = GetHTML()
-    if got != want {
-        t.Fatalf("got %s, want %s", got, want)
-    }
+	SetHTML("foo")
+	if got, want := GetHTML(), "foo"; got != want {
+		t.Fatalf("got noroute html %q want %q", got, want)
+	}
 }
diff --git a/registry/file/backend.go b/registry/file/backend.go
@@ -12,11 +12,19 @@ import (
 )
 
 func NewBackend(cfg *config.File) (registry.Backend, error) {
-	data, err := ioutil.ReadFile(cfg.Path)
+	routes, err := ioutil.ReadFile(cfg.RoutesPath)
 	if err != nil {
-		log.Println("[ERROR] Cannot read routes from ", cfg.Path)
+		log.Println("[ERROR] Cannot read routes from ", cfg.RoutesPath)
 		return nil, err
 	}
-	staticCfg := config.Static{cfg.NoRouteHTMLPath, string(data)}
-	return static.NewBackend(&staticCfg)
+	noroutehtml, err := ioutil.ReadFile(cfg.NoRouteHTMLPath)
+	if err != nil {
+		log.Println("[ERROR] Cannot read no route HTML from ", cfg.NoRouteHTMLPath)
+		return nil, err
+	}
+	staticCfg := &config.Static{
+		NoRouteHTML: string(noroutehtml),
+		Routes:      string(routes),
+	}
+	return static.NewBackend(staticCfg)
 }
diff --git a/registry/static/backend.go b/registry/static/backend.go
@@ -3,11 +3,7 @@
 package static
 
 import (
-	"io/ioutil"
-	"log"
-
 	"github.com/fabiolb/fabio/config"
-	"github.com/fabiolb/fabio/noroute"
 	"github.com/fabiolb/fabio/registry"
 )
 
@@ -45,13 +41,8 @@ func (b *be) WatchManual() chan string {
 	return make(chan string)
 }
 
-// WatchNoRouteHTML implementation that reads the noroute html from a
-// noroute.html file if it exists
 func (b *be) WatchNoRouteHTML() chan string {
-	data, err := ioutil.ReadFile(b.cfg.NoRouteHTMLPath)
-	if err != nil {
-		log.Printf("[WARN] Could not read NoRouteHTMLPath (%s)", b.cfg.NoRouteHTMLPath)
-	}
-	noroute.SetHTML(string(data))
-	return make(chan string)
+	ch := make(chan string, 1)
+	ch <- b.cfg.NoRouteHTML
+	return ch
 }