Migrate to gopkg.in/yaml.v3
#2468
Conversation
This replaces all usages of gopkg.in/yaml.v2 by v3 everywhere, adapts
the UnmarshalYAML methods to the new signature, and replaces the
UnmarshalStrict calls by the decoder with KnownFields flag.
util.YAMLMarshalUnmarshal doesn't work because it doesn't support
unmarshaling map[interface{}]interface{}
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
yaml.v3 can't unmarshal to map[interface{}]interface{},
but actually map[string]interface{} should be enough.
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
|
We can't use |
This commit is the latest v3 with go-yaml/yaml#691 cherry-picked on top of it, to provide support for strict unmarshaling when doing custom unmarshaling (see go-yaml/yaml#460) Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
There's no such field as `alertmanager_notification_limits_per_integration` and the fixed strict parsing has detected this. How does this test pass in `main`? Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
gopkg.in/yaml.v3 implementation doesn't seem to set the entire value to zero value if an empty node is specified in the YAML. So we don't need that check again, and we can adapt the test just to check that this assertion is true. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
|
I've forked |
| @@ -378,7 +380,7 @@ testuser: | |||
|
|
|||
| differentUserOverride := ` | |||
| differentuser: | |||
| alertmanager_notification_limits_per_integration: | |||
| alertmanager_notification_rate_limit_per_integration: | |||
There was a problem hiding this comment.
It took me a while to understand why the test was failing now 🤦. This test didn't do strict unmarshaling previously, so it wasn't really testing anything I guess.
There was a problem hiding this comment.
Excellent work!
I would suggest putting part of this comment into the PR description, because it might get hidden when commits are squashed:
gopkg.in/yaml.v3 implementation doesn't seem to set the entire value to
zero value if an empty node is specified in the YAML. So we don't need
that check again, and we can adapt the test just to check that this
assertion is true.
After upgrading to gopkg.in/yaml.v3 in [#2468], @pstibrany noticed that int 0 is no longer a valid time.Duration value. I've sent a PR to fix that: go-yaml/yaml#876 and cherry-picked the change into the branch we're using in Mimir as the replacement. [#2468]: #2468 Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
* Fix parsing int 0 as duration in yaml After upgrading to gopkg.in/yaml.v3 in [#2468], @pstibrany noticed that int 0 is no longer a valid time.Duration value. I've sent a PR to fix that: go-yaml/yaml#876 and cherry-picked the change into the branch we're using in Mimir as the replacement. [#2468]: #2468 Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Fix typo in the comment. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
What this PR does
Removes all usages of
gopkg.in/yaml.v2because it has some known bugs and we can't just useyaml.v3for parsing in some places becauseUnmarshalYAMLfunction signature isn't compatible between boths.I'd recommend reviewing the commits and their descriptions individually to get a better understanding of the changes.
Notes for the reviewers:
There's no
yaml.UnmarshalStrictanymore, so we need to create a new decoder and set theKnownFieldsflag, which is documented as:Additionally ,
gopkg.in/yaml.v3implementation doesn't seem to set the entire value to zero value if an empty node is specified in the YAML. So we don't need that check again, and we can adapt the test just to check that this assertion is true.Which issue(s) this PR fixes or relates to
Not fixing, but related to the common config efforts.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]