-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
#9635
Conversation
3a7b443
to
d3c51b0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9635 +/- ##
==========================================
+ Coverage 90.89% 90.99% +0.10%
==========================================
Files 347 349 +2
Lines 18324 18600 +276
==========================================
+ Hits 16655 16925 +270
- Misses 1344 1348 +4
- Partials 325 327 +2 ☔ View full report in Codecov by Sentry. |
2f769b6
to
0fffbeb
Compare
0fffbeb
to
bb5b428
Compare
…#31406) **Description:** Code changes required to make open-telemetry/opentelemetry-collector#9635 pass.
bd0f8f2
to
63231e7
Compare
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 working on this 🙇 🙇 I left a lot of comments, some probably stemming from my lack of familiarity with reflect
.
conf := New() | ||
if err := conf.Marshal(unmarshaler); err != nil { | ||
return nil, err | ||
} | ||
resultMap := conf.ToStringMap() | ||
for k, v := range resultMap { | ||
fromAsMap[k] = v | ||
} |
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.
- Comment: yes,sure can do
- oh no
- sure can do, I believe https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/confmap_test.go#L337 already covers this well.
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:
- On this PR, we add a new (failing) test for (2) and that uses
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. - On a future PR tackling Remove component.UnmarshalConfig #7102 we remove this
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.
…open-telemetry#31406) **Description:** Code changes required to make open-telemetry/opentelemetry-collector#9635 pass.
…ts. (open-telemetry#9635) **Description:** This implements support for calling `Unmarshal` on embedded structs of structs being decoded. **Link to tracking Issue:** Fixes open-telemetry#6671 **Testing:** Unit tests. Contrib fix is open: open-telemetry/opentelemetry-collector-contrib#31406
Description:
This implements support for calling
Unmarshal
on embedded structs of structs being decoded.Link to tracking Issue:
Fixes #6671
Testing:
Unit tests.
Contrib fix is open: open-telemetry/opentelemetry-collector-contrib#31406