Skip to content
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] - New merging mode to combine components #11046

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

VihasMakwana
Copy link
Contributor

Koanf's default merging strategy currently overrides static values such as slices, numbers, and strings. However, lists of components should be treated as a special case. This pull request introduces a new feature gate to allow for merging lists instead of discarding the existing ones.

With this new merging strategy:

  • Lists under the service configuration are merged rather than replaced.
  • The merging logic is name-aware, meaning that if components with the same name appear in both lists, they will only appear once in the final merged list.
  • This strategy only affects configurations under service::*.

Link to tracking issue

Related issues:

Testing

  • Added

Documentation

  • To be added

Note: I’d appreciate your feedback on this 🙏

@VihasMakwana VihasMakwana requested review from a team and codeboten September 4, 2024 11:07
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.18%. Comparing base (e99074d) to head (51d2a56).

Files with missing lines Patch % Lines
confmap/merge.go 82.50% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11046      +/-   ##
==========================================
- Coverage   92.20%   92.18%   -0.02%     
==========================================
  Files         405      406       +1     
  Lines       19244    19290      +46     
==========================================
+ Hits        17743    17782      +39     
- Misses       1134     1139       +5     
- Partials      367      369       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VihasMakwana
Copy link
Contributor Author

@codeboten @open-telemetry/collector-approvers I would appreciate any feedback you guys might have!

@mx-psi
Copy link
Member

mx-psi commented Sep 6, 2024

I think we need to discuss more on #8754 before moving forward with any concrete solution

@VihasMakwana
Copy link
Contributor Author

I think we need to discuss more on #8754 before moving forward with any concrete solution

Sure. Let's discuss more on #8754.

I'll move this PR to draft then.

@VihasMakwana VihasMakwana marked this pull request as draft September 6, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants