[confmap] confmap honors Unmarshal methods on config embedded structs.
#9635
Conversation
atoulme
force-pushed
the
support_embedded_structs_confmap
branch
3 times, most recently
from
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. |
atoulme
force-pushed
the
support_embedded_structs_confmap
branch
2 times, most recently
from
2f769b6
to
0fffbeb
Compare
atoulme
force-pushed
the
support_embedded_structs_confmap
branch
from
0fffbeb
to
bb5b428
Compare
…#31406) **Description:** Code changes required to make open-telemetry/opentelemetry-collector#9635 pass.
atoulme
force-pushed
the
support_embedded_structs_confmap
branch
from
bd0f8f2
to
63231e7
Compare
| 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.
Why do we need this? This is not done in unmarshalerHookFunc
There was a problem hiding this comment.
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.
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.
- 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.
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.Skipwith aTODOlinking 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
…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
Unmarshalon 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