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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[confmap] confmap honors
Unmarshal
methods on config embedded structs. #9635[confmap] confmap honors
Unmarshal
methods on config embedded structs. #9635Changes from 2 commits
bb5b428
63231e7
4e8a093
9ce3bb8
5096427
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? This is not done in
unmarshalerHookFunc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run after the main unmarshaler from
unmarshalerHookFunc
and must merge with the result, rather than set it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oki, can you add a comment about this? Both in the function itself as well as in mapstructure's configuration to make sure we don't change the order.
Also, I think it would be good to have a test that only triggers
unmarshalerHookFunc
and test that only triggers this hook (so, one struct with only embedded structs inside and one struct with no embedded structs inside in the tests).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.
is a big "oh no" because of #7102. We have a top level if/else that explicitly forks the flow if the struct implements Unmarshaler or not.If it does, it calls directly the Unmarshal function on the struct.
If it doesn't, it goes to mapstructure and then runs through the hooks.
This is this problem: #9635 (comment)
The workaround right now is that all structs with embedded structs that themselves implement Unmarshaler must themselves implement Unmarshaler and call mapstructure as part of their behavior.
We can straighten this up, but maybe in the next PR, if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining and being patient with me :) Here is my proposal on what to do:
t.Skip
with aTODO
linking to Remove component.UnmarshalConfig #7102. We also add a note on Remove component.UnmarshalConfig #7102 about said skip to not forget about it.t.Skip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added.