improve: possibility to provide id for external resource

[csviri]

PR introduces similar mechanism for external resource as for KubernetesDependetResource to easily override a method to provide an ID if user want't to avoid computing desired states multiple times.
Also improves the docs with pointers where we explain how to avoid calling desired multiple times.

[xstefank]

https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java#L269

Noting also that we should propably look into how to automate this with some kind of annotation comments in code to track correct line numbers.

[csviri]

yes, if we could solve that, would be awesome.

[xstefank]

@csviri but this link still needs to be fixed or not?

[csviri]

@metacosm I would like to merge this, we can have the desired caching as a followup pr on next. thx

[metacosm]

Reviewing this, I'm not sure I like the current design of putting the provider on the external resource class… I think it might be more appropriate to put it on the related dependent resource implementation since this is where the logic for this should be, imo. It doesn't feel right to put JOSDK-related logic on the external resource class, especially considering that maybe that class is not modifiable (if it's coming from a 3rd party library, for example). It also makes sense to gather the resource identification logic on the dependent resource implementation, imo.

[csviri]

Reviewing this, I'm not sure I like the current design of putting the provider on the external resource class… I think it might be more appropriate to put it on the related dependent resource implementation since this is where the logic for this should be, imo. It doesn't feel right to put JOSDK-related logic on the external resource class, especially considering that maybe that class is not modifiable (if it's coming from a 3rd party library, for example). It also makes sense to gather the resource identification logic on the dependent resource implementation, imo.

Yes, I was also thinking about that. Maybe we could generalize the cacheKeyMapper concept, see also generalization here:

#3007

WDYT?

But those are not necessarily contradicting nor we have to choose, we can have the (renamed to ResourceIDMapper ) cacheKeyMapper in dependent resources, where there will be still a posibility to use the ResourceIdProvider, just not limited to that.

[csviri]

So what I would do is to merge this and #3007 to next, but iterate over it for 5.2 to achieve what you also proposed.

[xstefank]

I think Chris has a point. So why it would be worth merging this now to next and then changing it later?

[csviri]

replaced by: #3007

@xstefank so I still intend to kepp the ExternalResourceVersionProvider just also use the CacheKeyMapper/ResourceIDMapper for DRs

let's continue the discussion under the linked PR, will prepare one with the mentioned changes

[metacosm]

Should the branch be deleted?