Repository: allegro/marathon-consul (go)

Issue:
Human-readable time durations in config files
It would be nice if we had human-readable time durations in config files instead of the nanosecond integers, for example:

``` json
"Sync": {
  "Interval": 900000000000
}
```

would become

``` json
"Sync": {
  "Interval": "15m"
}
```


Pull request description:
Fixes #56 | Humman-readable interval in config
Currently we allow user to specify intervals/timeouts in
human readable format only in command line flags. This commit
introduce allowign user to specify duration as inteager nanoseconds or
as formated string. String should have format supported by
time.ParseDuration.

Patch:
diff --git a/config/config.go b/config/config.go
index 5e45819..45f6a60 100644
--- a/config/config.go
+++ b/config/config.go
@@ -68,7 +68,7 @@ func (config *Config) parseFlags() {
 	flag.StringVar(&config.Consul.SslCaCert, "consul-ssl-ca-cert", "", "Path to a CA certificate file, containing one or more CA certificates to use to validate the certificate sent by the Consul server to us")
 	flag.StringVar(&config.Consul.Token, "consul-token", "", "The Consul ACL token")
 	flag.StringVar(&config.Consul.Tag, "consul-tag", "marathon", "Common tag name added to every service registered in Consul, should be unique for every Marathon-cluster connected to Consul")
-	flag.DurationVar(&config.Consul.Timeout, "consul-timeout", 3*time.Second, "Time limit for requests made by the Consul HTTP client. A Timeout of zero means no timeout")
+	flag.DurationVar(&config.Consul.Timeout.Duration, "consul-timeout", 3*time.Second, "Time limit for requests made by the Consul HTTP client. A Timeout of zero means no timeout")
 	flag.Uint32Var(&config.Consul.AgentFailuresTolerance, "consul-max-agent-failures", 3, "Max number of consecutive request failures for agent before removal from cache")
 	flag.Uint32Var(&config.Consul.RequestRetries, "consul-get-services-retry", 3, "Number of retries on failure when performing requests to Consul. Each retry uses different cached agent")
 	flag.StringVar(&config.Consul.ConsulNameSeparator, "consul-name-separator", ".", "Separator used to create default service name for Consul")
@@ -82,7 +82,7 @@ func (config *Config) parseFlags() {
 
 	// Sync
 	flag.BoolVar(&config.Sync.Enabled, "sync-enabled", true, "Enable Marathon-consul scheduled sync")
-	flag.DurationVar(&config.Sync.Interval, "sync-interval", 15*time.Minute, "Marathon-consul sync interval")
+	flag.DurationVar(&config.Sync.Interval.Duration, "sync-interval", 15*time.Minute, "Marathon-consul sync interval")
 	flag.StringVar(&config.Sync.Leader, "sync-leader", "", "Marathon cluster-wide node name (defaults to <hostname>:8080), the sync will run only if the specified node is the current Marathon-leader")
 	flag.BoolVar(&config.Sync.Force, "sync-force", false, "Force leadership-independent Marathon-consul sync (run always)")
 
@@ -92,12 +92,12 @@ func (config *Config) parseFlags() {
 	flag.StringVar(&config.Marathon.Username, "marathon-username", "", "Marathon username for basic auth")
 	flag.StringVar(&config.Marathon.Password, "marathon-password", "", "Marathon password for basic auth")
 	flag.BoolVar(&config.Marathon.VerifySsl, "marathon-ssl-verify", true, "Verify certificates when connecting via SSL")
-	flag.DurationVar(&config.Marathon.Timeout, "marathon-timeout", 30*time.Second, "Time limit for requests made by the Marathon HTTP client. A Timeout of zero means no timeout")
+	flag.DurationVar(&config.Marathon.Timeout.Duration, "marathon-timeout", 30*time.Second, "Time limit for requests made by the Marathon HTTP client. A Timeout of zero means no timeout")
 
 	// Metrics
 	flag.StringVar(&config.Metrics.Target, "metrics-target", "stdout", "Metrics destination stdout or graphite (empty string disables metrics)")
 	flag.StringVar(&config.Metrics.Prefix, "metrics-prefix", "default", "Metrics prefix (default is resolved to <hostname>.<app_name>")
-	flag.DurationVar(&config.Metrics.Interval, "metrics-interval", 30*time.Second, "Metrics reporting interval")
+	flag.DurationVar(&config.Metrics.Interval.Duration, "metrics-interval", 30*time.Second, "Metrics reporting interval")
 	flag.StringVar(&config.Metrics.Addr, "metrics-location", "", "Graphite URL (used when metrics-target is set to graphite)")
 
 	// Log
diff --git a/consul/agents.go b/consul/agents.go
index 002f9ca..a9d3a19 100644
--- a/consul/agents.go
+++ b/consul/agents.go
@@ -34,7 +34,7 @@ func NewAgents(config *Config) *ConcurrentAgents {
 				InsecureSkipVerify: !config.SslVerify,
 			},
 		},
-		Timeout: config.Timeout,
+		Timeout: config.Timeout.Duration,
 	}
 	return &ConcurrentAgents{
 		agents: make(map[string]*Agent),
diff --git a/consul/config.go b/consul/config.go
index 1d4cac1..7bb49d7 100644
--- a/consul/config.go
+++ b/consul/config.go
@@ -1,6 +1,6 @@
 package consul
 
-import "time"
+import "github.com/allegro/marathon-consul/time"
 
 type Config struct {
 	Auth                   Auth
@@ -11,7 +11,7 @@ type Config struct {
 	SslCaCert              string
 	Token                  string
 	Tag                    string
-	Timeout                time.Duration
+	Timeout                time.Interval
 	RequestRetries         uint32
 	AgentFailuresTolerance uint32
 	ConsulNameSeparator    string
diff --git a/debian/config.json b/debian/config.json
index b15a141..6a4fd5a 100644
--- a/debian/config.json
+++ b/debian/config.json
@@ -13,7 +13,7 @@
     "SslCaCert": "",
     "Token": "",
     "Tag": "marathon",
-    "Timeout": 3000000000,
+    "Timeout": "3s",
     "AgentFailuresTolerance": 3,
     "RequestRetries": 5,
     "IgnoredHealthChecks": ""
@@ -26,7 +26,7 @@
   },
   "Sync": {
     "Enabled": true,
-    "Interval": 900000000000,
+    "Interval": "15m0s",
     "Leader": "",
     "Force": false
   },
@@ -36,12 +36,12 @@
     "Username": "",
     "Password": "",
     "VerifySsl": true,
-    "Timeout": 30000000000
+    "Timeout": "30s"
   },
   "Metrics": {
     "Target": "stdout",
     "Prefix": "default",
-    "Interval": 30000000000,
+    "Interval": "30s",
     "Addr": ""
   },
   "Log": {
diff --git a/marathon/config.go b/marathon/config.go
index b7bff39..b40d6c8 100644
--- a/marathon/config.go
+++ b/marathon/config.go
@@ -1,6 +1,6 @@
 package marathon
 
-import "time"
+import "github.com/allegro/marathon-consul/time"
 
 type Config struct {
 	Location  string
@@ -8,5 +8,5 @@ type Config struct {
 	Username  string
 	Password  string
 	VerifySsl bool
-	Timeout   time.Duration
+	Timeout   time.Interval
 }
diff --git a/marathon/marathon.go b/marathon/marathon.go
index ed4c6fa..85151fe 100644
--- a/marathon/marathon.go
+++ b/marathon/marathon.go
@@ -51,7 +51,7 @@ func New(config Config) (*Marathon, error) {
 		Auth:     auth,
 		client: &http.Client{
 			Transport: transport,
-			Timeout:   config.Timeout,
+			Timeout:   config.Timeout.Duration,
 		},
 	}, nil
 }
diff --git a/metrics/config.go b/metrics/config.go
index 5b22018..00cdaf4 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -1,10 +1,10 @@
 package metrics
 
-import "time"
+import "github.com/allegro/marathon-consul/time"
 
 type Config struct {
 	Target   string
 	Prefix   string
-	Interval time.Duration
+	Interval time.Interval
 	Addr     string
 }
diff --git a/metrics/metrics.go b/metrics/metrics.go
index 7460423..0051bc0 100644
--- a/metrics/metrics.go
+++ b/metrics/metrics.go
@@ -49,14 +49,14 @@ func Init(cfg Config) error {
 	switch cfg.Target {
 	case "stdout":
 		log.Info("Sending metrics to stdout")
-		return initStdout(cfg.Interval)
+		return initStdout(cfg.Interval.Duration)
 	case "graphite":
 		if cfg.Addr == "" {
 			return errors.New("metrics: graphite addr missing")
 		}
 
 		log.Infof("Sending metrics to Graphite on %s as %q", cfg.Addr, pfx)
-		return initGraphite(cfg.Addr, cfg.Interval)
+		return initGraphite(cfg.Addr, cfg.Interval.Duration)
 	case "":
 		log.Infof("Metrics disabled")
 		return nil
diff --git a/sync/config.go b/sync/config.go
index 57ea4db..2aa1583 100644
--- a/sync/config.go
+++ b/sync/config.go
@@ -1,10 +1,10 @@
 package sync
 
-import "time"
+import "github.com/allegro/marathon-consul/time"
 
 type Config struct {
 	Enabled  bool
 	Force    bool
-	Interval time.Duration
+	Interval time.Interval
 	Leader   string
 }
diff --git a/sync/sync.go b/sync/sync.go
index 174362b..e3c11cf 100644
--- a/sync/sync.go
+++ b/sync/sync.go
@@ -37,7 +37,7 @@ func (s *Sync) StartSyncServicesJob() {
 		"Force":    s.config.Force,
 	}).Info("Marathon-consul sync job started")
 
-	ticker := time.NewTicker(s.config.Interval)
+	ticker := time.NewTicker(s.config.Interval.Duration)
 	go func() {
 		if err := s.SyncServices(); err != nil {
 			log.WithError(err).Error("An error occured while performing sync")
diff --git a/time/time.go b/time/time.go
new file mode 100644
index 0000000..4c4009b
--- /dev/null
+++ b/time/time.go
@@ -0,0 +1,51 @@
+package time
+
+import (
+	"encoding/json"
+	"strconv"
+	"strings"
+	"time"
+)
+
+type Timestamp struct {
+	time.Time
+}
+
+func (t *Timestamp) UnmarshalJSON(b []byte) (err error) {
+	s := strings.Trim(string(b), "\"")
+	if s == "null" {
+		t.Time = time.Time{}
+		return
+	}
+	t.Time, err = time.Parse(time.RFC3339Nano, s)
+	return
+}
+
+func (t Timestamp) MarshalJSON() ([]byte, error) {
+	return json.Marshal(t.String())
+}
+
+func (t *Timestamp) String() string {
+	return t.Format(time.RFC3339Nano)
+}
+
+type Interval struct {
+	time.Duration
+}
+
+func (t *Interval) UnmarshalJSON(b []byte) (err error) {
+	s := strings.Trim(string(b), "\"")
+	if s == "null" {
+		t.Duration = 0
+		return nil
+	} else if value, e := strconv.ParseInt(s, 10, 64); e == nil {
+		t.Duration = time.Duration(value)
+		return nil
+	}
+	t.Duration, err = time.ParseDuration(s)
+	return err
+}
+
+func (t Interval) MarshalJSON() ([]byte, error) {
+	return json.Marshal(t.Duration.String())
+}


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 2759575..5d679cc 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/allegro/marathon-consul/marathon"
 	"github.com/allegro/marathon-consul/metrics"
 	"github.com/allegro/marathon-consul/sync"
+	timeutil "github.com/allegro/marathon-consul/time"
 	"github.com/allegro/marathon-consul/web"
 	"github.com/stretchr/testify/assert"
 )
@@ -96,7 +97,7 @@ func TestConfig_ShouldBeMergedWithFileDefaultsAndFlags(t *testing.T) {
 			SslCaCert:              "",
 			Token:                  "",
 			Tag:                    "marathon",
-			Timeout:                3 * time.Second,
+			Timeout:                timeutil.Interval{Duration: 3 * time.Second},
 			RequestRetries:         5,
 			AgentFailuresTolerance: 3,
 			ConsulNameSeparator:    ".",
@@ -108,7 +109,7 @@ func TestConfig_ShouldBeMergedWithFileDefaultsAndFlags(t *testing.T) {
 			MaxEventSize: 4096,
 		},
 		Sync: sync.Config{
-			Interval: 15 * time.Minute,
+			Interval: timeutil.Interval{Duration: 15 * time.Minute},
 			Enabled:  true,
 			Leader:   "",
 			Force:    false,
@@ -118,10 +119,10 @@ func TestConfig_ShouldBeMergedWithFileDefaultsAndFlags(t *testing.T) {
 			Username:  "",
 			Password:  "",
 			VerifySsl: true,
-			Timeout:   30 * time.Second},
+			Timeout:   timeutil.Interval{Duration: 30 * time.Second}},
 		Metrics: metrics.Config{Target: "stdout",
 			Prefix:   "default",
-			Interval: 30 * time.Second,
+			Interval: timeutil.Interval{Duration: 30 * time.Second},
 			Addr:     ""},
 		Log: struct{ Level, Format, File string }{
 			Level:  "info",
diff --git a/consul/consul_test.go b/consul/consul_test.go
index 16bfd8b..768e714 100644
--- a/consul/consul_test.go
+++ b/consul/consul_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/allegro/marathon-consul/apps"
 	"github.com/allegro/marathon-consul/service"
+	timeutil "github.com/allegro/marathon-consul/time"
 	"github.com/allegro/marathon-consul/utils"
 	consulapi "github.com/hashicorp/consul/api"
 	"github.com/stretchr/testify/assert"
@@ -28,7 +29,7 @@ func TestGetAgent_FullConfig(t *testing.T) {
 
 	// given
 	agents := NewAgents(&Config{Token: "token", SslEnabled: true,
-		Auth: Auth{Enabled: true, Username: "", Password: ""}, Timeout: time.Second})
+		Auth: Auth{Enabled: true, Username: "", Password: ""}, Timeout: timeutil.Interval{Duration: time.Second}})
 
 	// when
 	agent, err := agents.GetAgent("127.23.23.23")
@@ -651,7 +652,7 @@ func TestAddAgentsFromApp(t *testing.T) {
 
 	// create consul without any agents in cache
 	consul := New(Config{
-		Timeout:             10 * time.Millisecond,
+		Timeout:             timeutil.Interval{Duration: 10 * time.Millisecond},
 		Port:                fmt.Sprintf("%d", server.Config.Ports.HTTP),
 		ConsulNameSeparator: ".",
 		Tag:                 "marathon",
diff --git a/consul/consul_test_server.go b/consul/consul_test_server.go
index a59c5de..9e1bb17 100644
--- a/consul/consul_test_server.go
+++ b/consul/consul_test_server.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 	"time"
 
+	timeutil "github.com/allegro/marathon-consul/time"
 	"github.com/hashicorp/consul/testutil"
 	"github.com/stretchr/testify/assert"
 )
@@ -61,7 +62,7 @@ func FailingClient() *Consul {
 
 func consulClientAtAddress(host string, port int) *Consul {
 	config := Config{
-		Timeout:             10 * time.Millisecond,
+		Timeout:             timeutil.Interval{Duration: 10 * time.Millisecond},
 		Port:                fmt.Sprintf("%d", port),
 		ConsulNameSeparator: ".",
 	}
diff --git a/sync/sync_test.go b/sync/sync_test.go
index 043d8de..363cfbd 100644
--- a/sync/sync_test.go
+++ b/sync/sync_test.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/allegro/marathon-consul/apps"
 	"github.com/allegro/marathon-consul/service"
+	timeutil "github.com/allegro/marathon-consul/time"
 )
 
 var noopSyncStartedListener = func(apps []*apps.App) {}
@@ -26,7 +27,7 @@ func TestSyncJob_ShouldSyncOnLeadership(t *testing.T) {
 	services := newConsulServicesMock()
 	sync := New(Config{
 		Enabled:  true,
-		Interval: 10 * time.Millisecond,
+		Interval: timeutil.Interval{Duration: 10 * time.Millisecond},
 		Leader:   "current.leader:8080",
 	}, marathon, services, noopSyncStartedListener)
 
@@ -46,7 +47,7 @@ func TestSyncJob_ShouldNotSyncWhenDisabled(t *testing.T) {
 	services := newConsulServicesMock()
 	sync := New(Config{
 		Enabled:  false,
-		Interval: 10 * time.Millisecond,
+		Interval: timeutil.Interval{Duration: 10 * time.Millisecond},
 		Leader:   "current.leader:8080",
 	}, marathon, services, noopSyncStartedListener)
 
@@ -67,7 +68,7 @@ func TestSyncJob_ShouldDefaultLeaderConfigurationToResolvedHostname(t *testing.T
 	services := newConsulServicesMock()
 	sync := New(Config{
 		Enabled:  true,
-		Interval: 10 * time.Millisecond,
+		Interval: timeutil.Interval{Duration: 10 * time.Millisecond},
 	}, marathon, services, noopSyncStartedListener)
 
 	// when
diff --git a/time/time_test.go b/time/time_test.go
new file mode 100644
index 0000000..db1f52f
--- /dev/null
+++ b/time/time_test.go
@@ -0,0 +1,46 @@
+package time
+
+import (
+	"encoding/json"
+	"github.com/stretchr/testify/assert"
+	"testing"
+	gotime "time"
+)
+
+func TestTimestampParsing(t *testing.T) {
+	t.Parallel()
+	in := "2014-03-01T23:29:30.158Z"
+	bytes, err := json.Marshal(in)
+	assert.NoError(t, err)
+	var out Timestamp
+	err = json.Unmarshal(bytes, &out)
+	assert.NoError(t, err)
+	assert.Equal(t, in, out.String())
+}
+
+func TestDurationIntegerParsing(t *testing.T) {
+	t.Parallel()
+	var out Interval
+	err := json.Unmarshal([]byte("900000000000"), &out)
+	assert.NoError(t, err)
+	assert.Equal(t, "15m0s", out.String())
+}
+
+func TestInvalidDurationParsing(t *testing.T) {
+	t.Parallel()
+	var out Interval
+	err := json.Unmarshal([]byte(`"72xyz"`), &out)
+	assert.Error(t, err)
+	assert.Equal(t, "0s", out.String())
+}
+
+func TestDurationMarshal(t *testing.T) {
+	t.Parallel()
+	var out Interval
+	out.Duration, _ = gotime.ParseDuration("15m")
+	bytes, err := json.Marshal(out)
+	assert.NoError(t, err)
+	err = json.Unmarshal(bytes, &out)
+	assert.NoError(t, err)
+	assert.Equal(t, "15m0s", out.String())
+}