PR comments fixups.

config/config.go

@@ -14,11 +14,13 @@
 package config
 
 import (
-  "time"
+	"fmt"
+	"time"
 
 	"code.google.com/p/goprotobuf/proto"
 
 	pb "github.com/prometheus/alert_manager/config/generated"
+
 	"github.com/prometheus/alert_manager/manager"
 )
 
@@ -37,22 +39,31 @@ func (c Config) String() string {
 
 // Validate checks an entire parsed Config for the validity of its fields.
 func (c Config) Validate() error {
-  // BUG: Nothing to do here for now.
+	for _, a := range c.AggregationRule {
+		for _, f := range a.Filter {
+			if f.NameRe == nil {
+				return fmt.Errorf("Missing name pattern (name_re) in filter definition: %s", proto.MarshalTextString(f))
+			}
+			if f.ValueRe == nil {
+				return fmt.Errorf("Missing value pattern (value_re) in filter definition: %s", proto.MarshalTextString(f))
+			}
+		}
+	}
 	return nil
 }
 
 // Rules returns all the AggregationRules in a Config object.
 func (c Config) AggregationRules() manager.AggregationRules {
-  rules := make(manager.AggregationRules, 0, len(c.AggregationRule))
+	rules := make(manager.AggregationRules, 0, len(c.AggregationRule))
 	for _, r := range c.AggregationRule {
-    filters := make(manager.Filters, 0, len(r.Filter))
-    for _, filter := range r.Filter {
-      filters = append(filters, manager.NewFilter(filter.GetNameRe(), filter.GetValueRe()))
-    }
+		filters := make(manager.Filters, 0, len(r.Filter))
+		for _, filter := range r.Filter {
+			filters = append(filters, manager.NewFilter(filter.GetNameRe(), filter.GetValueRe()))
+		}
 		rules = append(rules, &manager.AggregationRule{
-      Filters: filters,
-      RepeatRate: time.Duration(r.GetRepeatRate()) * time.Second,
-    })
+			Filters:    filters,
+			RepeatRate: time.Duration(r.GetRepeatRateSeconds()) * time.Second,
+		})
 	}
 	return rules
 }

config/config.proto

@@ -16,14 +16,14 @@ package io.prometheus.alert_manager;
 // A regex-based label filter used in aggregations.
 message Filter {
   // The regex matching the label name.
-  required string name_re = 1;
+  optional string name_re = 1;
   // The regex matching the label value.
-  required string value_re = 2;
+  optional string value_re = 2;
 }
 
 message AggregationRule {
   repeated Filter filter = 1;
-  optional int32 repeat_rate = 2 [default = 7200];
+  optional int32 repeat_rate_seconds = 2 [default = 7200];
 }
 
 message AlertManagerConfig {

config/config_test.go

@@ -21,34 +21,44 @@ import (
 
 var fixturesPath = "fixtures"
 
-var configTests = []struct {
+type configTest struct {
 	inputFile   string
 	shouldFail  bool
 	errContains string
-}{
-	{
-		inputFile: "empty.conf.input",
-	}, {
-		inputFile: "sample.conf.input",
-	}, {
-		inputFile:   "invalid_proto_format.conf.input",
-		shouldFail:  true,
-		errContains: "unknown field name",
-	},
 }
 
-func TestConfigs(t *testing.T) {
-	for i, configTest := range configTests {
-		_, err := LoadFromFile(path.Join(fixturesPath, configTest.inputFile))
+func (ct *configTest) test(i int, t *testing.T) {
+	_, err := LoadFromFile(path.Join(fixturesPath, ct.inputFile))
 
-		if err != nil {
-			if !configTest.shouldFail {
-				t.Fatalf("%d. Error parsing config %v: %v", i, configTest.inputFile, err)
-			} else {
-				if !strings.Contains(err.Error(), configTest.errContains) {
-					t.Fatalf("%d. Expected error containing '%v', got: %v", i, configTest.errContains, err)
-				}
+	if err != nil {
+		if !ct.shouldFail {
+			t.Fatalf("%d. Error parsing config %v: %v", i, ct.inputFile, err)
+		} else {
+			if !strings.Contains(err.Error(), ct.errContains) {
+				t.Fatalf("%d. Expected error containing '%v', got: %v", i, ct.errContains, err)
 			}
 		}
 	}
 }
+
+func TestConfigs(t *testing.T) {
+	var configTests = []configTest{
+		{
+			inputFile: "empty.conf.input",
+		}, {
+			inputFile: "sample.conf.input",
+		}, {
+			inputFile:   "missing_name_re.conf.input",
+			shouldFail:  true,
+			errContains: "Missing name pattern",
+		}, {
+			inputFile:   "invalid_proto_format.conf.input",
+			shouldFail:  true,
+			errContains: "unknown field name",
+		},
+	}
+
+	for i, ct := range configTests {
+		ct.test(i, t)
+	}
+}

config/fixtures/sample.conf.input

@@ -7,7 +7,7 @@ aggregation_rule {
     name_re: "zone"
     value_re: "aa"
   }
-  repeat_rate: 3600
+  repeat_rate_seconds: 3600
 }
 
 aggregation_rule {

config/generated/config.pb.go

@@ -14,8 +14,8 @@ var _ = &json.SyntaxError{}
 var _ = math.Inf
 
 type Filter struct {
-	NameRe           *string `protobuf:"bytes,1,req,name=name_re" json:"name_re,omitempty"`
-	ValueRe          *string `protobuf:"bytes,2,req,name=value_re" json:"value_re,omitempty"`
+	NameRe           *string `protobuf:"bytes,1,opt,name=name_re" json:"name_re,omitempty"`
+	ValueRe          *string `protobuf:"bytes,2,opt,name=value_re" json:"value_re,omitempty"`
 	XXX_unrecognized []byte  `json:"-"`
 }
 
@@ -38,16 +38,16 @@ func (m *Filter) GetValueRe() string {
 }
 
 type AggregationRule struct {
-	Filter           []*Filter `protobuf:"bytes,1,rep,name=filter" json:"filter,omitempty"`
-	RepeatRate       *int32    `protobuf:"varint,2,opt,name=repeat_rate,def=7200" json:"repeat_rate,omitempty"`
-	XXX_unrecognized []byte    `json:"-"`
+	Filter            []*Filter `protobuf:"bytes,1,rep,name=filter" json:"filter,omitempty"`
+	RepeatRateSeconds *int32    `protobuf:"varint,2,opt,name=repeat_rate_seconds,def=7200" json:"repeat_rate_seconds,omitempty"`
+	XXX_unrecognized  []byte    `json:"-"`
 }
 
 func (m *AggregationRule) Reset()         { *m = AggregationRule{} }
 func (m *AggregationRule) String() string { return proto.CompactTextString(m) }
 func (*AggregationRule) ProtoMessage()    {}
 
-const Default_AggregationRule_RepeatRate int32 = 7200
+const Default_AggregationRule_RepeatRateSeconds int32 = 7200
 
 func (m *AggregationRule) GetFilter() []*Filter {
 	if m != nil {
@@ -56,11 +56,11 @@ func (m *AggregationRule) GetFilter() []*Filter {
 	return nil
 }
 
-func (m *AggregationRule) GetRepeatRate() int32 {
-	if m != nil && m.RepeatRate != nil {
-		return *m.RepeatRate
+func (m *AggregationRule) GetRepeatRateSeconds() int32 {
+	if m != nil && m.RepeatRateSeconds != nil {
+		return *m.RepeatRateSeconds
 	}
-	return Default_AggregationRule_RepeatRate
+	return Default_AggregationRule_RepeatRateSeconds
 }
 
 type AlertManagerConfig struct {

config/load.go

@@ -14,10 +14,11 @@
 package config
 
 import (
+	"io/ioutil"
+
 	"code.google.com/p/goprotobuf/proto"
 
 	pb "github.com/prometheus/alert_manager/config/generated"
-	"io/ioutil"
 )
 
 func LoadFromString(configStr string) (Config, error) {
@@ -26,7 +27,7 @@ func LoadFromString(configStr string) (Config, error) {
 		return Config{}, err
 	}
 
-	config := Config{configProto}
+	config := Config{AlertManagerConfig: configProto}
 	err := config.Validate()
 
 	return config, err

main.go

@@ -32,7 +32,7 @@ func main() {
 
 	conf, err := config.LoadFromFile(*configFile)
 	if err != nil {
-		log.Fatalf("Error loading configuration from %s: %v", *configFile, err)
+		log.Fatalf("Error loading configuration from %s: %s", *configFile, err)
 	}
 
 	suppressor := manager.NewSuppressor()