promtool: alert_rule_test items require alertname #8504
Conversation
|
LGTM -- this addresses the confusion seen in #8462 (that PR can be closed now). |
cmd/promtool/unittest.go
Outdated
| @@ -188,6 +188,9 @@ func (tg *testGroup) test(evalInterval time.Duration, groupOrderMap map[string]i | |||
| // Map of all the unit tests for given eval_time. | |||
| alertTests := make(map[model.Duration][]alertTestCase) | |||
| for _, alert := range tg.AlertRuleTests { | |||
| if alert.Alertname == "" { | |||
| return []error{errors.Errorf("An item under alert_rule_test misses required attribute alertname at eval_time %v", alert.EvalTime)} | |||
There was a problem hiding this comment.
can we log the test group name, if it is set?
There was a problem hiding this comment.
Also, formatting nit: error messages should not be capitalized, i.e. please start with a lower case letter.
There was a problem hiding this comment.
@beorn7 I think that nit is applying the go rule a bit too broadly; given we know where the error is used here it's ok to use capitalisation, as the main reason for the rule is an error might be wrapped, but here we're inside a main package and know it will be printed out, so it looks ugly to not capitalise the error message.
There was a problem hiding this comment.
I'm not so much following the Go convention here but what is done elsewhere in this file.
I'm open to capitalize the messages, but then you have to rework the other error messages, too.
(Generally, I'd suggest to do that in a separate PR.)
Accepting alert_rule_test without alertname is confusing as it will always pass with empty exp_alerts, and never with non-empty exp_alerts. Signed-off-by: Raphael Bauduin <raphael.bauduin@tessares.net>
|
thanks!! |
Accepting alert_rule_test without alertname is confusing as it will
always pass with empty exp_alerts, and never with non-empty exp_alerts.