DecodeWithOptions method for Node #691
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
colega
approved these changes
Jul 20, 2022
|
@niemeyer is there any chance to get this merged? #460 is a blocker for us to adopt v3, while v2 has bugs that require us to move away from it. |
colega
added a commit
to grafana/mimir
that referenced
this pull request
Jul 20, 2022
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>
3 tasks
colega
added a commit
to grafana/mimir
that referenced
this pull request
Jul 20, 2022
* Use gopkg.in/yaml.v3 everywhere
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>
* Fix yaml.YAMLMarshalUnmarshal
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>
* go mod tidy
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
* Add faillint for gopkg.in/yaml.v2
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
* Use custom fork of gopkg.in/yaml.v3
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>
* Fix TestAlertmanagerNotificationLimitsOverrides
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>
* Remove cfg.validateYAMLEmptyNodes and adapt test
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 think this is nice and I would also love this to be merged in. The only issue left from #460 is that you cannot know inside |
|
@niemeyer WDYT? Thanks very much. it's a great PR. |
|
Ping @niemeyer , is there anything missing for this to get merged? I'm up for even cutting a new PR as you prefer if that's the case, I'm really missing this feature. PS: I doodled with an alternative interface here https://github.com/fornellas/yaml/tree/node_known_fields. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested fix for #460.
In my opinion it's the most straightforward way to fix the issue while keeping backwards compatibility and considering the small number of options (currently only one).
Obviously open for discussion.