fix: make explicit that Updater extends Matcher #1786
Conversation
| import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result; | ||
|
|
||
| public interface Updater<R, P extends HasMetadata> { | ||
| public interface Updater<R, P extends HasMetadata> extends Matcher<R, P> { |
There was a problem hiding this comment.
I'm not sure we want to do this. Matcher is something we are just using somewhere in Kubernetes Dependent Resource, where it makes sense to configure different kind of matchers. But that servers completely different purpose.
What purpose it would it serve having it here?
There was a problem hiding this comment.
I understand what you mean but this isn't actually true since Updater uses Matcher.Result so to be really coherent, we should extract Result from Matcher. That said, having the Matcher interface here doesn't really hurt, imo, and makes it explicit that Updater needs to provide that exact same behavior. It also makes it clearer that classes that extend an Updater (e.g. KubernetesDependentResource) don't really need to implement Matcher and can just override the provided implementation (which is the root cause of #1783).
There was a problem hiding this comment.
It just confusing, so basically if the abstraction is not used anywhere directly, we should not define that abstraction. Is anywhere this Matcher referenced in the code? Is this useful somewhere? IMO this is kinda anti-pattern, at least I don't see what purpose it serves.
so to be really coherent, we should extract Result from Matcher
that is the point, it does not need to be coherent.
I think this is the same issue as we do with LifecycleAware , it was defined because it was nice, but actually it is never referenced, then it is limiting if something does not fit. But again that abstraction is not explicitly used to abstract away a problem
There was a problem hiding this comment.
In my opinion, the reverse is actually happening as evidenced by #1783: people think they need to implement Matcher when they actually don't need to because the current code makes it implicit and difficult to figure out. Making Updater extends Matcher makes things more explicit. It also solves the issue that Updater depends on Matcher for no obvious reason in the current code while providing the exact same behavior.
There was a problem hiding this comment.
We can check explicitly also that if implements matcher and throw an exception if does. I think is much clear if we don't compose interfaces with no real reason, and just have the method in update. But changing the code because the documentation is not read by developers and that it accindetally creates an issue is not a good argument to change the design. Well it is an argument, but would rather stick with the current one, and check the revers, document it better.
There was a problem hiding this comment.
Not saying that having this matcher in Updater would be very bad or something, then just we have to probably create BulkMatcher, and those 2 are API changes actually that we should consider.
There was a problem hiding this comment.
I don't agree that there is no reason to compose the interfaces: the Updater needs to perform the match behavior, so it makes sense that it extends the Matcher interface.
There was a problem hiding this comment.
I mean no strong opinon if it is done also to bulkmatcher elegantly, and released as part of minor (not patch)
|
SonarCloud Quality Gate failed. |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
github-actions
bot
added
the
lifecycle/stale
metacosm
removed
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
openshift-ci
bot
added
lifecycle/stale
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
No description provided.