-
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] Remove top level condition when considering struct as Unmarshalers. #9862
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9862 +/- ##
==========================================
- Coverage 91.88% 91.86% -0.03%
==========================================
Files 360 360
Lines 16728 16732 +4
==========================================
Hits 15371 15371
- Misses 1020 1022 +2
- Partials 337 339 +2 ☔ View full report in Codecov by Sentry. |
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.
It would also be interesting to try and reproduce the failing contrib test on a unit test here, so we can work on it. The offending |
**Description:** Adds two tests to confmap to test some edge cases **Link to tracking Issue:** Written while reviewing #9862
added a skipped test here: TestRecursiveUnmarshaling |
8637f2f
to
5ad613b
Compare
If you want a foolproof solution, we might have to do something a little more drastic. We might need to keep a hashset of all pointers of unmarshaler methods ever called, to make sure we don't run into cycles. |
I guess we can forbid what the stanza package does instead. Do you think there is a benefit to being able to do what stanza does? I can't see any advantage right now |
No, I don't see any advantage, except maybe if you're trying to unmarshal and set all values on the struct atomically. Even then, this is a corner case. I think we're 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.
I am okay with the current approach, I guess we should document the limitation about nested unmarshalling before merging, but I think we can go with this.
cc @open-telemetry/collector-approvers, I would appreciate a second pair of eyes since this is somewhat subtle. Even if this is ready before, I will wait at least until Monday to merge to give others some time to review.
One more PR in contrib needed: open-telemetry/opentelemetry-collector-contrib#32577 |
**Description:** Update contrib to match core with the latest changes that are needed for open-telemetry/opentelemetry-collector#9862
a5083b2
to
9afa5ef
Compare
The last contrib issue is solved by open-telemetry/opentelemetry-collector-contrib#32660. Unfortunately, I couldn't find a good way to fix this - in effect, a breaking change. |
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.
Looks good to me, I think this is probably the cleanest solution given the current state of things.
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.
LGTM after adressing Evan's comment. This will need an update on contrib since it's breaking the tests there
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
9afa5ef
to
236bfa0
Compare
@atoulme are these expected:
|
@atoulme So that we can merge this soon and address @codeboten's comment, would you be able to do the following?
|
…2577) **Description:** Update contrib to match core with the latest changes that are needed for open-telemetry/opentelemetry-collector#9862
Here is the PR open-telemetry/opentelemetry-collector-contrib#32660 It is past midnight, I will follow up tomorrow on it. |
The contrib PR shows the changes combined with test changes to contrib will work. I think we have reached step 2. I have reached out to Alex to make sure he's ok with this. |
@codeboten are you ok with this approach and the steps I have followed, as well as the checks they provide? Can we merge this PR? |
Can we agree to merge this by end of next week unless someone objects? |
I'll put a reminder to merge in a wekk, @codeboten feel free to merge before then if you are okay with it :) |
…shalers. (open-telemetry#9862) This is a slice of open-telemetry#9750 focusing on removing the top level condition on unmarshaling structs. --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
…shalers. (open-telemetry#9862) This is a slice of open-telemetry#9750 focusing on removing the top level condition on unmarshaling structs. --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
This is a slice of #9750 focusing on removing the top level condition on unmarshaling structs.