Yaml UnmarshalStrict implementation.#4033
Conversation
krasi-georgiev
left a comment
There was a problem hiding this comment.
best to add one more test to check for duplicated fields.
config/config.go
Outdated
| } | ||
|
|
||
| return yaml_util.CheckOverflow(c.XXX, "remote_write") | ||
| return nil |
There was a problem hiding this comment.
return c.HTTPClientConfig.Validate()
config/config.go
Outdated
| } | ||
|
|
||
| return yaml_util.CheckOverflow(c.XXX, "remote_read") | ||
| return nil |
There was a problem hiding this comment.
return c.HTTPClientConfig.Validate()
|
noticed that https://github.com/prometheus/common/tree/master/config also uses the |
Few small nits.
simplify if-err statments.
|
LGTM waiting for one more LGTM and will merge. |
|
There seems to be quite a few unrelated cleanup changes in here. Considering the breadth of this change, I'd prefer this be limited to just the core change. |
|
all changes are related to the UnmarshalStrict usage. |
|
No, there's quite a few other changes in here. |
|
can you put a comment next to the line you think is unrelated and will double check. |
|
There are multiple unrelated changes to error handling and scoping. |
|
I reverted some of the changes, if there are any more to be reverted please let me know which ones when you have the time. Thank you. |
10c506d to
0786d1f
Compare
| } | ||
| case ".yml", ".yaml": | ||
| if err := yaml.Unmarshal(content, &targetGroups); err != nil { | ||
| if err := yaml.UnmarshalStrict(content, &targetGroups); err != nil { |
There was a problem hiding this comment.
Looks like we don't have unittests for this, can you add one?
There was a problem hiding this comment.
I added a static configuration check for yaml files, sorry for taking too long to push.
|
Thanks! |
* Updated yaml vendor package. * remove checkOverflow duplicate in rulefmt * remove duplicated HTTPClientConfig.Validate() * Added yaml static check.
This resolves #4017.
As suggested, I updated configuration to call
UnmarshalStrict()over simpleunmarshalling, thus rendering
util/yaml/yaml.gono longer needed.The original vendor package
gopkg.in/yaml.v2was updated and used (https://godoc.org/gopkg.in/yaml.v2#UnmarshalStrict).Tests ran successfully against
config/and usingmake testwith go1.10.Edit: As @krasi-georgiev pointed out, this also fixes #1275.