fix: make it possible to have read-only dependents

[metacosm]

Fixes #955

[csviri]

Isn't read only dependent just an informer?

I would not change the AbstractDependentResource, would rather extend DependentResource and have ReadOnlyDependetResource - that would have empty reconcile method. Basically just wrap an informer to getting a resource.
That could be done quite in general way also for other informers.

[metacosm]

Isn't read only dependent just an informer?

I would not change the AbstractDependentResource, would rather extend DependentResource and have ReadOnlyDependetResource - that would have empty reconcile method. Basically just wrap an informer to getting a resource. That could be done quite in general way also for other informers.

I knew you would say that and I don't agree. 😄
The class hierarchy is already deep enough and there's no need to create another class when this can be solved by just changing the return type of the desired method.

[csviri]

Isn't read only dependent just an informer?
I would not change the AbstractDependentResource, would rather extend DependentResource and have ReadOnlyDependetResource - that would have empty reconcile method. Basically just wrap an informer to getting a resource. That could be done quite in general way also for other informers.

I knew you would say that and I don't agree. smile The class hierarchy is already deep enough and there's no need to create another class when this can be solved by just changing the return type of the desired method.

This should be about the right abstractions. It would not be deeper, wider, this is why we have DependentResource this abstract in first place. So if we want to have read only dependent resources I think we should reflect that in our mode, like this is the domain language. AbstractReadOnlyDependetResource would be really usefull for other and most of it can be implemented in general. Basically just the getResource needs to be implemented, in the abstract. So that would be very clean.

[scrocquesel]

Isn't read only dependent just an informer?
I would not change the AbstractDependentResource, would rather extend DependentResource and have ReadOnlyDependetResource - that would have empty reconcile method. Basically just wrap an informer to getting a resource. That could be done quite in general way also for other informers.

I knew you would say that and I don't agree. smile The class hierarchy is already deep enough and there's no need to create another class when this can be solved by just changing the return type of the desired method.

This should be about the right abstractions. It would not be deeper, wider, this is why we have DependentResource this abstract in first place. So if we want to have read only dependent resources I think we should reflect that in our mode, like this is the domain language. AbstractReadOnlyDependetResource would be really usefull for other and most of it can be implemented in general. Basically just the getResource needs to be implemented, in the abstract. So that would be very clean.

If it looks like a dependent but don't behave like a dependent, then it is not a dependent. I think a definition of what is a dependent resource should be settled. To me Dependent is too generic. Does it mean a resource the primary resource reconciler :

• manage the lifecycle
• modify but do not own the lifecycle
• needs to read to behave on other dependents

From what I understand of the actual DependentResource interface, it has been narrowed to be only the first case. And I'm not convince, it will be a one fit all. Maybe better to be prepared to handle other kinds ?

Even if it is abstract away with noop default, it is weird to have a reconcile and delete method and resource that don't need it. To me, it is more like a LookupResource that can be added to dependents ?

@ControllerConfiguration(finalizerName = NO_FINALIZER,
    dependents = {
        @Lookup(type = MyLookupResource.class),
        @Dependent(type = MyDependentResource.class),
        @Edit(type = MyEditableResource.class
    })

interface LookupResource {
    getResource(T primary);
}

interface EditResource {
    getResource(T primary);

    apply(...);
    undo(...);

    ...
}

[csviri]

I agree with this basically. This is also a matter of definition, what is a dependent resource, but this when needs to just read is not really a dependent resource, true. At least not feels like it. It could be defined as a super interface actually to the dependent resource, maybe just having getResource(). Basically that it is, what we need from it.

The one reason that on DependetResource there is a reconcile method, is to able to cover cases which are not exactly an "ideal" dependent resource. For example the editable feels more like this. So covering that as a abstraction as EditedDependentResource what extends DependentResource is reasonable still IMHO.

So right modeling is essential here. @metacosm wants to limit the hierarchy and that is not a bad aspect of the problem. On the other hand we should capture these notions in the domain model. And for now it does not seems very extreme with the mentioned abstractions. (see model below)

I think first we should think in object models, then have an annotation structure defined/handled as secondary, since that is limited. There are problem which are essentially harder to do in the background. I would like to have this api with annotations, ideally this whole structure is declarative, but that should be the second step. Like for example in standalone mode if we need a status of a dependent resource as an input of an other it really can just reference the dependent resource as an attribute in class. For annotations we need additional mechanisms in the background what gives more complexity.

So would propose:

classDiagram
    ReadableResource <|-- DependentResource
    DependentResource <|-- AbstractDependentResource
    DependentResource <|-- EditableResource     
    AbstractDependentResource <|-- KuberentesDependentResource

Loading

But I don't have strong opinion on ReadableResource. This could be a subclass of DependentResource. Just really don't want to hack everythinh under AbstractDependentResource, then DependentResource abstraction does not even make sense.

Maybe AbstractDependentResource coudl be renamed to GenericDependentResource.
(ps. mermaid diagrams are very cool :) )

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
1 Security Hotspot
21 Code Smells

28.1% Coverage
0.6% Duplication

[metacosm]

The deeper the class hierarchy the more fragile the architecture becomes and the more complex it is to configure things via annotations. So yes, a read-only Kubernetes dependent is essentially an informer but this allows to very quickly create an informer using simply an annotation instead of figuring out which class to use among 5-6 of them and have to manually wire it.

Of note, the initial implementation used to split the different behaviors by interfaces where create, update, delete were traits that you could add to your dependent.

[csviri]

Maybe it's just me, but for me is much more easy to understand a clean class hierarchy then check why a functionality is hacked to an abstraction what serves a completely different purpose. Like from now on if somebody wants to create a read only resource (for an external resource let's say) will have extend implement AbstractDependentResource, override create and update methods with empty body since those are abstract. For me this is very very confusing.

In addition to that now every KubernetesDependentResource desire method override will return an Optional. And this is mandatory to override in every case. What is also seems to smelly.

Also why it is harder to configure? it's anyway a new subclass, where just a config needs to be passed.

Can provide an alternate implementation to show how I mean, we can compare it. With this approach, how I meant it originally:

classDiagram    
    DependentResource <|-- GenericDependentResource
    DependentResource <|-- EditableResource     
    DependentResource <|-- ReadOnlyDependentResource
    GenericDependentResource <|-- KuberentesDependentResource

Loading

Having probably an AbstractKubernetesResource to have a subclass for versions of Kubernernetes resoruces.

I agree that this has it's negative sides too. So maybe it's really make sense to compare prototypes.
But the goal is also to have it easily extendable for external resources. As we have with the EventSources (Polling, PerResource polling). Then it's good to have these notions there explicitly.

Nevertheless this is an interesting topic, so thx to initiate and starting on it.

[scrocquesel]

Of note, the initial implementation used to split the different behaviors by interfaces where create, update, delete were traits that you could add to your dependent.

Isn't the reconciler v2 refactoring all about using trait interface ?

Class hierarchy are a pain when we tends to put things in the hierarchy that should not. But the same fragility will happens when building fat class. Here, Interface Segregation principle is violated and to me, while it seems to be a quick win now, it certainly become a pain latter.

Maybe it's just me, but for me is much more easy to understand a clean class hierarchy then check why a functionality is hacked to an abstraction what serves a completely different purpose. Like from now on if somebody wants to create a read only resource (for an external resource let's say) will have extend implement AbstractDependentResource, override create and update methods with empty body since those are abstract. For me this is very very confusing.

Confusing enough, delete on the parent interface has been forgotten too.

I agree that this has it's negative sides too. So maybe it's really make sense to compare prototypes. But the goal is also to have it easily extendable for external resources. As we have with the EventSources (Polling, PerResource polling). Then it's good to have these notions there explicitly.

Nevertheless this is an interesting topic, so thx to initiate and starting on it.

I like your last diagram. Even if for me DependentResource may be an empty marker interface to ease configuration.

Looking at DependentResourceManager, it is just a delegation pattern over a collection. Maybe complexity should be there. Manager is the glue between the prepareEventSources/reconcile/cleanup logic and the specific behavior of the resource itself. Maybe their should be a hierachy in there to handle, Generic, Editable, ReadOnly.

Roughly,

DependentResourceManager<Generic> {
    // as actual.
}

EditableResourceManager<EditableResource> {
    reconcile(P primary){
      dependent.getResource().ifPresent(editable -> dependent.apply(editable));
    }
    cleanup(P primary){
      dependent.getResource().ifPresent(editable -> dependent.undo(editable));
    }
}

ReadOnlyDependentResourceManager<ReadOnlyDependentResource> {
    reconcile(P primary){
    }
    cleanup(P primary){
    }
}

[csviri]

Created a prototype with class hierarchy:
#958

It ended up quite ok. Especially when for a polling (readonly) dependent resource. But when created an abstract Kuberentes Resource, turned out also quite reasonable for the sub-classes, and it's quite clear.

The editable at the end added as a feature flag to the KubernetesDependentResource since it's just about to turning on/off one part of it.

Pls take a look, what do you think about this way.

[csviri]

Isn't the reconciler v2 refactoring all about using trait interface ?

Not really, but there were changes regarding that, too. It was more about how we address resources in event sources, using informers for main event source (also for caching) etc.

Looking at DependentResourceManager, it is just a delegation pattern over a collection. Maybe complexity should be there. Manager is the glue between the prepareEventSources/reconcile/cleanup logic and the specific behavior of the resource itself. Maybe their should be a hierachy in there to handle, Generic, Editable, ReadOnly.

One of the design aspects however is to able to use the DependentResources in standalone mode:
#887
Moving logic into separate managers contradicts with this as far I can see.

[metacosm]

Replaced by #963.