Skip to content

Conversation

florianknecht
Copy link

AbstractEventSourceHolderDependentResource may have the current state of the dependent resource in its event sources cache. Let's use that by default.

Otherwise the super implementation in AbstractDependentResource runs empty via context.getSecondaryResources, never looking for information in the event source's cache. Which forces all inheritors to implement this lookup themselves (e.g. PerResourcePollingExternalDependentResource, where I noticed this).

@openshift-ci openshift-ci bot requested review from csviri and xstefank September 25, 2025 09:46
@florianknecht florianknecht force-pushed the eventsource-for-secondaryresource branch from 2c662f0 to c885980 Compare September 25, 2025 09:58
@florianknecht florianknecht force-pushed the eventsource-for-secondaryresource branch from c885980 to cc462c2 Compare September 25, 2025 09:59
@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

This is a bot problematic, since the target event source could still hold multiple resources for this type.
This if indeed better for the case where are multiple event sources for the same type.
But would need to add also this check:

var targetResources = secondaryResources.stream().filter(r -> r.equals(desired)).toList();

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

Note that the current design check on equals what is not ideal, since it is easy to miss that users need to override that, so in future we might have a more explicit solution for that.

@florianknecht
Copy link
Author

Got it - digging a little deeper with your hint made me understand how it's supposed to work. Thanks a lot!
I think I found something else though - will add a new PR for discussion.

@florianknecht florianknecht deleted the eventsource-for-secondaryresource branch September 25, 2025 10:52
@metacosm
Copy link
Collaborator

Note that the current design check on equals what is not ideal, since it is easy to miss that users need to override that, so in future we might have a more explicit solution for that.

The current design is wrong because it will never properly match resources that need to be updated even if equals is overridden (because in this case, equals will return false anyway).

@florianknecht
Copy link
Author

florianknecht commented Sep 25, 2025

My issues here were caused by my Kotlin runtime not enforcing checked exceptions, see #2965

@metacosm you're right, while a secondary resource is not reconciled it will not be returned by this method. Might be intentional though - I've not yet gotten deep enough into this. On the plus side, Kotlin DataClasses already implement a clean equals/hashcode for my external secondary resource type so I got that covered at least.

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

The current design is wrong because it will never properly match resources that need to be updated even if equals is overridden (because in this case, equals will return false anyway).

I checked this now on mysql sample, and it finds the resource, I see no issue there. Could you elaborate pls?

see:

image

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

@florianknecht
note that if equals does not work for you, you are always free to override selectTargetSecondaryResource:

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

@florianknecht
create a draft with the rough idea I had on mind to improve this:
#2970
If this provides a nicer API, pls let us know, maybe we can continue discussion there.

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

But the current approach is also functional, I don't see it broken. The API might be more elegant, though.

@metacosm
Copy link
Collaborator

The current design is wrong because it will never properly match resources that need to be updated even if equals is overridden (because in this case, equals will return false anyway).

I checked this now on mysql sample, and it finds the resource, I see no issue there. Could you elaborate pls?

see:
image

Have you tested with a scenario where the dependent resource needs to be updated (i.e. the current state doesn't match the desired state)?

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

The current design is wrong because it will never properly match resources that need to be updated even if equals is overridden (because in this case, equals will return false anyway).

I checked this now on mysql sample, and it finds the resource, I see no issue there. Could you elaborate pls?
see:
image

Have you tested with a scenario where the dependent resource needs to be updated (i.e. the current state doesn't match the desired state)?

That dependent resources is NOT an Updater so by definition not doing updates,see:

implements ConfiguredDependentResource<ResourcePollerConfig>,
Creator<Schema, MySQLSchema>,
Deleter<MySQLSchema> {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants