-
Notifications
You must be signed in to change notification settings - Fork 220
feat: expose event source metadata #1617
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
Conversation
Could you pls make a summary what is the problem being solved with this PR? |
|
||
private final EventSource original; | ||
private final String name; | ||
private final AssociatedDependentMetadata dependentMetadata; |
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.
Porbably not fully understand the PR, but really don't like this part. The event sources should have no knowledge at all about dependent resources.
|
||
import io.javaoperatorsdk.operator.processing.event.source.EventSource; | ||
|
||
public interface EventSourceMetadata { |
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.
Shouldn't we just simply expose NamedEventSource
?
This is not really metadata, it is THE event source with name.
Or rename it to EventSourceWithMetadata
.
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 debated only using NamedEventSource
, don't recall why I didn't actually… 😅
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.
Ah yes, didn't want to expose start
/ stop
but since the EventSource
is exposed… 🤷🏼
The alternative would be to expose only what's needed from the EventSource
via another interface.
Kudos, SonarCloud Quality Gate passed! |
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.
LGMT
No description provided.