fix: add onUpdateFilterOr option to the Informer#3436
Conversation
Signed-off-by: xstefank <xstefank122@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an “OR” update-filter option to informer/controller event filtering so controllers can still be generation-aware while explicitly allowing additional update events (e.g., specific status-path updates done via SSA), addressing #3413.
Changes:
- Introduces
onUpdateFilterOron@Informerand propagates it throughInformerConfiguration/InformerEventSourceConfiguration. - Updates
ControllerEventSourceto combine filters as(internal AND onUpdateFilter) OR onUpdateFilterOr. - Adds unit/integration tests covering the new OR-filter behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/filter/OrUpdateFilter.java | Adds a test OnUpdateFilter used to validate OR-filter behavior. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/filter/OrFilterTestReconciler.java | Configures a controller with @Informer(onUpdateFilterOr = ...) for integration testing. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/filter/OrFilterIT.java | Integration test validating OR-filter can trigger reconciliation on updates that internal filters would reject. |
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSourceTest.java | Unit tests for OR-filter behavior and updated controller test wiring. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java | Implements the effective filter composition with optional OR-filter support. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerEventSourceConfiguration.java | Adds builder support and propagation for withOnUpdateFilterOr. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/InformerConfiguration.java | Adds storage, builder methods, and annotation initialization for onUpdateFilterOr. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/informer/Informer.java | Adds onUpdateFilterOr() to the annotation and documents intended behavior. |
| * Optional {@link OnUpdateFilter} combined with JOSDK's internal filters using OR logic — the | ||
| * event is accepted when either this filter or JOSDK's internal filters accept it. Use this to | ||
| * expand the set of events that trigger reconciliation beyond what JOSDK's internal filters (e.g. | ||
| * generation-aware filtering) would normally allow, for instance to also reconcile on specific | ||
| * status field updates. |
| await() | ||
| .pollDelay(Duration.ofMillis(POLL_DELAY)) | ||
| .untilAsserted(() -> assertThat(reconciler().getNumberOfExecutions()).isEqualTo(3)); |
|
Maybe it would make more sense to have a switch to change the composition behavior from AND to OR instead of defining another filter with a different behavior? |
|
@xstefank I see more conceptual issues with this approach. One is what @metacosm mentioned:
and yes I think too, that would be one way to solve this. Another approach would be to have a flag to "withoutDefaultFilters", so default filters don't interfere with the users, so user would have a full control of the filters, that would be future-proof, also might impose a simpler mental model. In addition to that this should not be on Informer level, since it only make sense in context of |
|
Another approach would be to expose the internal filters and provide ways to let users combine them as they want. That would be future-proof but maybe a little overkill. |
Exactly, this is how I meant also, so if users set the flag to not add the default filter, they will set a custom filter(s), but there they could just combine those filters with the existing one in any way they want. But I think they can do this even now, we just don't have the way to "not add" the default ones. |
|
Replaced with #3437 |
|
do you think that withoutDefaultFilters + verifying that users can still set up the same filter as they have by default would still be a valid use case? Now with #3437, they can switch defaultFilter OR userFilters. It is a slightly different scenario. |
Fixes #3413