redesign #906
redesign #906
Conversation
metacosm
commented
Feb 3, 2022
metacosm
commented
- fix: fail explicitly if current namespace is requested but not available
- fix: retain annotation for reflective access
- feat: add desired and matches methods on DependentResource
...r-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DependentResource.java
Outdated
Show resolved
Hide resolved
| * the specified primary resource, {@code false} otherwise | ||
| */ | ||
| default boolean match(R actual, P primary, Context context) { | ||
| return true; |
There was a problem hiding this comment.
shouldn't we use equals here?
There was a problem hiding this comment.
No, see the javadoc 😉
There was a problem hiding this comment.
for typical Kubernetes resource implementations, simply calling
* {@code desired(primary, context).equals(actual)} is not enough because metadata will usually be
* different.
This is true, but in a default implementation I would just compare specs. And in reconcile, would merge the target resource with the actual from the server. Thus just override the spec, add other fields if new to metadata (and override if some is generated). Would also make this behavior customizable (already is, some can override), to make a custom comaparator.
* The default implementation always return {@code true}, which corresponds to the behavior where
* the dependent never needs to be updated after it's been created.
Why should be this the default?
There was a problem hiding this comment.
This is true, but in a default implementation I would just compare specs. And in reconcile, would merge the target resource with the actual from the server. Thus just override the spec, add other fields if new to metadata (and override if some is generated). Would also make this behavior customizable (already is, some can override), to make a custom comaparator.
That's a good point
Why should be this the default?
Why shouldn't it? 😄
More seriously what do you think the default behavior should be? Not updating the resource after it's been created sounds like a reasonable default which covers a lot of use cases (for example, pretty much all our samples), imo.
There was a problem hiding this comment.
The default should be to generate the target default with desired, and compare the specs.
The reason is that I think the default use case should be what we have in WebPage (what actally needs some cleanup), but basically that the reconciler manages all the resources. That is not that strong assumption IMHO, sure there are cases where not, but this would be quite sensible default.
|
|
||
| private final Builder<R, P> builder; | ||
| private final Updater<R, P> updater; | ||
| private final Cleaner<R, P> cleaner; | ||
| private final Persister<R, P> persister; |
There was a problem hiding this comment.
Yes for non-Kubernetes resources, see the MySQL sample
There was a problem hiding this comment.
Not sure if it make sense for kubernetes objects; maybe we should have this just implicit in the reconcile function. Or just have it for a subclass for non-kubernetes objects and get called from reconcile in that impl.
There was a problem hiding this comment.
Kubernetes resources get one automatically
There was a problem hiding this comment.
We could probably remove the Persister interface but I think that would require people defining Kubernetes dependents to either extend a class that would provide the behavior as it exists right now or do more work to implement the methods that are currently in Persister.
...src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java
Outdated
Show resolved
Hide resolved
...r-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DependentResource.java
Outdated
Show resolved
Hide resolved
| * (or ever) | ||
| */ | ||
| default Optional<R> desired(P primary, Context context) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
I think it should not have a default implementation.
The most common case is like in the WebPage, when all the resources are managed by the operator, IMHO that should be the default approach. If the resource is not managed by us (so not created and not updated) should be probably a feature flag/mode on the resources. Maybe eventually a different type of resource.
There was a problem hiding this comment.
I'm not sure I follow. If we're not creating, nor updating the resource, why would it even be a dependent?
There was a problem hiding this comment.
To get input from it, like not sure from nodes or other configurations of the cluster. But something like this happens also in the tomcat example too if remember correctly.
|
Made one more comment, I think we can merge it. Made a comment, but probably the best appraoch would be to fine tune it in a subsequent PRs. |
This removes the need for Builder and Updater interfaces.
|
SonarCloud Quality Gate failed. |
