Validate Slack field config and only allow the necessary input#1334
Validate Slack field config and only allow the necessary input#1334stuartnelson3 merged 1 commit intoprometheus:masterfrom
Conversation
config/notifiers.go
Outdated
| return err | ||
| } | ||
| if c.Title == "" { | ||
| return fmt.Errorf("missing title in Slack field configuration, value=%v", c.Value) |
There was a problem hiding this comment.
I wouldn't log c.Value in the error.
config/notifiers.go
Outdated
| return fmt.Errorf("missing title in Slack field configuration, value=%v", c.Value) | ||
| } | ||
| if c.Value == "" { | ||
| return fmt.Errorf("missing value in Slack field configuration, title=%v", c.Title) |
| } | ||
| } | ||
|
|
||
| func TestSlackFieldConfigValidation(t *testing.T) { |
There was a problem hiding this comment.
Can you add a test case with a valid YAML connfiguration?
notify/impl.go
Outdated
| for index, field := range n.conf.Fields { | ||
| // Check if short was defined for the field otherwise fallback to the global setting | ||
| var short bool | ||
| if field.Short { |
There was a problem hiding this comment.
This won't work as expected when field.Short is set to false on purpose. Short needs to be a *bool instead.
taharah
force-pushed
the
fix-slack-config
branch
3 times, most recently
from
1e64169 to
7197159
Compare
|
@simonpasquier I've resolved all of the comments you made. Thank you for reviewing! |
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
2 small remarks, LGTM otherwise.
| value: hello | ||
| short: true | ||
| - title: second | ||
| value: world |
There was a problem hiding this comment.
It would be good to validate that the unmarshaled data is matching what is expected when there's no error.
There was a problem hiding this comment.
👍 I'll get this added.
notify/impl.go
Outdated
| var fields = make([]config.SlackField, numFields) | ||
| for index, field := range n.conf.Fields { | ||
| // Check if short was defined for the field otherwise fallback to the global setting | ||
| var short *bool |
There was a problem hiding this comment.
nit but I would do:
var short bool
if field.Short != nil {
short = *field.Short
} else {
short = n.conf.ShortFields
}There was a problem hiding this comment.
The main reason it is being set as it is currently is that I reused SlackField from the config. package, which has Short defined as a *bool now. I initially had it like that, but changed it after the tests failed and caught the error. I can change the if statement to be as above, and then pass a pointer to the variable short when building the SlackField.
taharah
force-pushed
the
fix-slack-config
branch
from
7197159 to
ddb8032
Compare
Signed-off-by: Trevor Wood <Trevor.G.Wood@gmail.com>
taharah
force-pushed
the
fix-slack-config
branch
from
ddb8032 to
4d046e1
Compare
|
@simonpasquier I resolved your latest comments. If there's anything else you see in the latest batches of changes let me know and I'll get them pushed up. Thanks! |
|
LGTM |
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
3 participants
Resolves the comments on the PR #1002 to update the documentation for Slack fields. Currently, any arbitrary key/value pairs can be defined; however, per the Slack documentation for defining fields [1] only the properties
title,value, andshortcan be defined with the first two being required for each field. This updates the Slack field configuration to only allow the aforementioned three properties as well as validates that both a title and value are defined for each property in theyaml.Unmarshalerinterface. Moreover, this allows the value forshortto be defined on a per-field basis.