feat: explicit event filters for all event sources #1294
Conversation
csviri
changed the title
| * trigger reconciliation - since if finalizer used there the event is an update that triggers the | ||
| * cleanup. If not used the resources are cleaned up by garbage collector.) | ||
| **/ | ||
| Class<? extends Predicate<? extends HasMetadata>> onAddFilter() default VoidOnAddFilter.class; |
There was a problem hiding this comment.
I think we should have our own interface for this so that it's easier to evolve it if needed and easier to track (i.e. easier to find the usages of our own class than a JDK class)…
There was a problem hiding this comment.
I don't have strong opinion on this. The only thing that is not nice on that is we need to implement our own operations for add and or. In addition to that this is not one interface, then it is three interfaces for every operation, because those have different parameters. And note that we are already passing all the input from the upstream api (informer, even source), so not sure if that API is somewhere to evolve.
| Class<? extends ResourceEventFilter>[] eventFilters() default {}; | ||
|
|
||
| /** | ||
| * Filter of onAdd events of resources. (Note that onDelete is missing, delete events don't |
There was a problem hiding this comment.
The comment on onDelete shouldn't be here, imo, but in the docs.
There was a problem hiding this comment.
Hmm, kk, actually was thinking if this needs explicit docs, since it is quite straightforward, so javadoc might be enough. But had to put the delete part somewhere. Can remove it, and maybe just add an FAQ?
...k-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java
Outdated
Show resolved
Hide resolved
| private boolean namespacesWereConfigured = false; | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| protected Predicate onAddFilter; |
There was a problem hiding this comment.
This is not typesafe and can lead to hard to diagnose errors.
There was a problem hiding this comment.
No, the thing is it comes from annotations in this case, can make paramtrized class, at the end itt will not help with that problem, since in the annotation eventually are no generics (KubernetesDependent). But at least on this layer might look better, true.
|
|
||
| public EventSource initEventSource(EventSourceContext<P> context) { | ||
| // some sub-classes (e.g. KubernetesDependentResource) can have their event source created | ||
| // before this method is called in the managed case, so only create the event source if it | ||
| // hasn't already been set | ||
| // hasn't already been set. | ||
| // The filters are applied automatically only if event source is created automatically. |
There was a problem hiding this comment.
So if the event source is configured explicitly (think like sharing an event source between multiple dependent resources) it does not override the shared event source filters.
There was a problem hiding this comment.
Will add this to the comment.
| @@ -15,4 +22,18 @@ protected AbstractResourceEventSource(Class<R> resourceClass) { | |||
| public Class<R> resourceType() { | |||
| return resourceClass; | |||
| } | |||
|
|
|||
| public void setOnAddFilter(Predicate<R> onAddFilter) { | |||
There was a problem hiding this comment.
I don't think that the set methods should be exposed.
There was a problem hiding this comment.
This is for every event source, so also the external resources ones, will check, maybe to hide here and make it visible there.
...re/src/main/java/io/javaoperatorsdk/operator/processing/event/source/CachingEventSource.java
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed! |
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
No description provided.