Implement strict yaml unmarshalling.#1318
Conversation
|
@simonpasquier can you have a look if you have a minute as I am stuck with some other issues. |
config/config.go
Outdated
| return fmt.Errorf("missing name in receiver") | ||
| } | ||
| return checkOverflow(c.XXX, "receiver config") | ||
| return unmarshal((*plain)(c)) |
There was a problem hiding this comment.
L551-553 shouldn't be removed and return nil here.
config/notifiers.go
Outdated
| c.Headers = normalizedHeaders | ||
|
|
||
| return checkOverflow(c.XXX, "email config") | ||
| return unmarshal((*plain)(c)) |
config/notifiers.go
Outdated
| return fmt.Errorf("missing service or routing key in PagerDuty config") | ||
| } | ||
| return checkOverflow(c.XXX, "pagerduty config") | ||
| return unmarshal((*plain)(c)) |
config/notifiers.go
Outdated
| } | ||
|
|
||
| return checkOverflow(c.XXX, "hipchat config") | ||
| return unmarshal((*plain)(c)) |
config/notifiers.go
Outdated
| return fmt.Errorf("missing Wechat CorpID in Wechat config") | ||
| } | ||
| return checkOverflow(c.XXX, "Wechat config") | ||
| return unmarshal((*plain)(c)) |
config/notifiers.go
Outdated
| return fmt.Errorf("missing Routing key in VictorOps config") | ||
| } | ||
| return checkOverflow(c.XXX, "victorops config") | ||
| return unmarshal((*plain)(c)) |
config/notifiers.go
Outdated
| return fmt.Errorf("missing token in Pushover config") | ||
| } | ||
| return checkOverflow(c.XXX, "pushover config") | ||
| return unmarshal((*plain)(c)) |
| cannonicalName := strings.Replace(name, "\\", "/", -1) | ||
| return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...) | ||
| } | ||
|
|
There was a problem hiding this comment.
This change needs to be reverted.
| @@ -392,19 +392,19 @@ func AssetNames() []string { | |||
|
|
|||
There was a problem hiding this comment.
This change needs to be reverted.
|
@simonpasquier I reverted all of the unnecessary formatting and refactoring changes as you suggested. |
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
One minor remark otherwise this is good for me.
config/config_test.go
Outdated
| ` | ||
| conf := &Config{} | ||
| err := yaml.Unmarshal([]byte(in), conf) | ||
| err := yaml.UnmarshalStrict([]byte(in), conf) |
There was a problem hiding this comment.
It would be good to use the same pattern as in the other tests:
- conf := &Config{}
- err := yaml.Unmarshal([]byte(in), conf)
+ _, err := Load(in)
There was a problem hiding this comment.
I changed it. It was doing the same job predefined in Load, so there is no need for it. Thanks!
|
@manosf you need to sign all the commits IIUC. |
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
LGTM, thanks! You still need to sign the commits to comply with DCO (the easier is probably to squash them all).
|
@simonpasquier Done, thanks! |
|
Can you resolve the conflict? |
|
@stuartnelson3 I just removed the file from staging since it no longer uses yaml unmarshalling. |
Signed-off-by: manosf <manosf@protonmail.com>
4 participants
As with prometheus/prometheus#4033 and prometheus/common/pull/127, I updated the yaml vendor package in order to implement strict yaml unmarshalling. Thus, checkOverflow() is no longer needed since UnmarshalStrict() covers duplicate and unknown fields in yaml files.
Also some cleanup after deleting some -no longer necessary- variables and checkOverflow() calls.
cc @krasi-georgiev