-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Minimum changes to component.Host, to allow split of the component package #6553
Conversation
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.
Just one question, one way I could imagine doing this gracefully is by providing a new HostWithComponent
interface and deprecating Host
in favour of it, any thoughts?
@codeboten the idea behind this draft PR was mostly to agree on the path forward that it is possible to split |
I'm ok with this plan, assuming others are as well. I think the changes to Host are minimal enough and help us untangle all the other components into more meaningful packages. I'm in favour of #6552 |
efa0d4d
to
6906dc2
Compare
Codecov ReportBase: 90.94% // Head: 90.91% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #6553 +/- ##
==========================================
- Coverage 90.94% 90.91% -0.04%
==========================================
Files 241 241
Lines 14025 14077 +52
==========================================
+ Hits 12755 12798 +43
- Misses 1021 1027 +6
- Partials 249 252 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Do we to remove |
Extension is the only one that needs to stay as a type. |
943c008
to
0ef1531
Compare
Can you please point me to why? I'm not sure splitting |
Not sure I understand, I said that only "Extension" needs to stay as a type since we don't have a |
0ef1531
to
259dda8
Compare
Signed-off-by: Bogdan Drutu <[email protected]>
259dda8
to
d49f88c
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.
Could you also please provide some reasoning behind this change in the PR description
Co-authored-by: Dmitrii Anoshin <[email protected]>
4dd2a8f
to
fed87b8
Compare
This is the first step towards moving the [Receiver|Processor|Exporter|Extension] specific parts of the components package to its corresponding packages as defined in #6578. For that, we need to remove the
component.Host
dependency oncomponent.Exporter
andcomponent.Extension
.Since
component.[Exporter|Receiver|Processor]
are useless interface given that wecomponent.[Traces|Metrics|Log][Exporter|Receiver|Processor]
exist, they can be simply removed.component.Extension
should not be removed, butGetExtensions()
method ofcomponent.Host
interface has to be changed tocomponent.Host.GetExtensions() map[component.ID]component.Component
to remove dependency oncomponent.Extension
. In this PR, it's done through aliasing, so it's not a breaking change. But, later, we will need to remove the aliasing, so make sure to update your implementations ofcomponent.Host.GetExtensions()
fromto