feat: move controller informer-related configuration to InformerConfig by metacosm · Pull Request #2455 · operator-framework/java-operator-sdk

metacosm · 2024-06-25T12:42:29Z

No description provided.

Signed-off-by: Chris Laprun <claprun@redhat.com>

csviri

Made some changes, I think we should discuss mainly some naming and API issues mentioned, mainly the one related to the followControllerNamespacesOnChange.

Shall we just document that, that it is a problem. Maybe we can check and throw an exception if that is set explicitly for the controller?!

csviri · 2024-07-04T11:41:13Z

+            .withInformerConfiguration(c -> c.withLabelSelector(SELECTOR))
            .build(), context);
    var ingressEventSource =
        new InformerEventSource<>(InformerConfiguration.from(Ingress.class, WebPage.class)


This feels little weird, maybe just naming issue but

InformerConfiguration.from(..).withInformerConfiguraiton(...) somehow does not sounds right.

Here we could just simply hide the holder, and put back the with* methods that would result a simpler / cleaner API.

The idea is to reuse the same code at some point to deal with the InformerConfig… and yes, the naming is sub-optimal, which is why I'd like to clean things up but I'd rather do that in a subsequent PR

I mean, we could just let the old methods, like here, the withLabelSelector will be in the background the InformerConfigHolder. Would be good to have this in this PR, this is something I would not release for sure. But if you insist feel free to do separately.

I think I had tried that (or some variation of it) but maybe I didn't. Let me see what it does.

I would re-add these methods after we're done with renaming and cleaning-up if we feel it's necessary.

csviri · 2024-07-04T13:04:27Z

@@ -21,56 +24,60 @@
  String name() default NO_VALUE_SET;


name does not makes too much sense for the controller either :(

I wondered about this, actually, if we could somehow reuse the same config for the controller and some other event source but yes, I agree that this doesn't make much sense for the controller.

The problem is that we cannot inherit between annotations so either:

we have separate annotations that do more or less the same thing with code to process each that will again more or less do the same thing, which will cause quite a big maintenance load

we accept that some of the setup doesn't make sense for both, add proper safeguards and live with it

we extract the fields that don't make sense in a different annotation for event sources that will enclose an InformerConfig field but that adds yet another level of annotations, which would be less than ideal.

Exactly, imo 1. or 2.
Not sure if that is that much more code to have a @ControllerInformer.

Or alternative is also just simply stick with the current approach and don't merge this PR.

I think I would prefer 2. at this point, with more clean up on the naming, it should be much better, I just wanted to control the blast radius at this point :)

metacosm · 2024-07-04T14:08:20Z

Made some changes, I think we should discuss mainly some naming and API issues mentioned, mainly the one related to the followControllerNamespacesOnChange.

I don't see any changes. Did you push them?

Shall we just document that, that it is a problem. Maybe we can check and throw an exception if that is set explicitly for the controller?!

Maybe.

metacosm · 2024-07-08T13:56:11Z

Thanks, I will open a subsequent PR to clean things up and rename things more appropriately.

#2455) * feat: move controller informer-related configuration to InformerConfig Signed-off-by: Chris Laprun <claprun@redhat.com> * refactor: start isolating ResourceConfiguration Signed-off-by: Chris Laprun <claprun@redhat.com> * fix: initFromAnnotation now properly inits the current instance Signed-off-by: Chris Laprun <claprun@redhat.com> * fix: default onDeleteFilter implementation Signed-off-by: Chris Laprun <claprun@redhat.com> * fix: properly set default namespaces in controller case Signed-off-by: Chris Laprun <claprun@redhat.com> * refactor: remove KubernetesDependentInformerConfigBuilder Signed-off-by: Chris Laprun <claprun@redhat.com> * refactor: use InformerConfigHolder in more places, unifying handling Signed-off-by: Chris Laprun <claprun@redhat.com> * fix: properly propagate name to informer config Signed-off-by: Chris Laprun <claprun@redhat.com> * feat: add factory method to init builder from an extising configuation Signed-off-by: Chris Laprun <claprun@redhat.com> * fix: remove potentially problematic default implementation Signed-off-by: Chris Laprun <claprun@redhat.com> --------- Signed-off-by: Chris Laprun <claprun@redhat.com>

metacosm self-assigned this Jun 25, 2024

metacosm requested a review from csviri June 25, 2024 12:42

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2024

metacosm force-pushed the controller-informer branch 2 times, most recently from 1b8aae5 to 4c776aa Compare June 26, 2024 12:14

metacosm marked this pull request as ready for review June 27, 2024 20:01

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2024

openshift-ci Bot requested review from adam-sandor and andreaTP June 27, 2024 20:01

metacosm force-pushed the controller-informer branch from 309d3b7 to db3b7e1 Compare July 1, 2024 11:11

metacosm added 10 commits July 2, 2024 22:30

feat: move controller informer-related configuration to InformerConfig

a951b46

Signed-off-by: Chris Laprun <claprun@redhat.com>

refactor: start isolating ResourceConfiguration

865844f

Signed-off-by: Chris Laprun <claprun@redhat.com>

fix: initFromAnnotation now properly inits the current instance

da296e2

Signed-off-by: Chris Laprun <claprun@redhat.com>

fix: default onDeleteFilter implementation

72fc123

Signed-off-by: Chris Laprun <claprun@redhat.com>

fix: properly set default namespaces in controller case

72737ea

Signed-off-by: Chris Laprun <claprun@redhat.com>

refactor: remove KubernetesDependentInformerConfigBuilder

51a5534

Signed-off-by: Chris Laprun <claprun@redhat.com>

refactor: use InformerConfigHolder in more places, unifying handling

8227897

Signed-off-by: Chris Laprun <claprun@redhat.com>

fix: properly propagate name to informer config

80c952f

Signed-off-by: Chris Laprun <claprun@redhat.com>

feat: add factory method to init builder from an extising configuation

e99899a

Signed-off-by: Chris Laprun <claprun@redhat.com>

fix: remove potentially problematic default implementation

99ca7e2

Signed-off-by: Chris Laprun <claprun@redhat.com>

metacosm force-pushed the controller-informer branch from 261e5f5 to 99ca7e2 Compare July 2, 2024 20:30

csviri requested changes Jul 4, 2024

View reviewed changes

csviri reviewed Jul 4, 2024

View reviewed changes

metacosm mentioned this pull request Jul 5, 2024

Clean up InformerConfiguration and related classes #2465

Closed

metacosm requested a review from csviri July 5, 2024 13:12

csviri approved these changes Jul 8, 2024

View reviewed changes

metacosm merged commit 534daaf into next Jul 8, 2024

metacosm deleted the controller-informer branch July 8, 2024 13:56

csviri linked an issue Jul 8, 2024 that may be closed by this pull request

Use @InformerConfig in @ControllerConfiguration #2447

Closed

csviri mentioned this pull request Aug 14, 2024

improve: replace jenvtest with kube-api-test from fabric8 client #2497

Merged

csviri mentioned this pull request Sep 17, 2024

feat: context getSecondary resource is activation condition aware #2532

Merged

This was referenced Dec 3, 2024

chore: fabri8 client to v7.0.0 #2616

Merged

docs: migration guide as blogpost refactor #2618

Closed

Uh oh!

Conversation

metacosm commented Jun 25, 2024

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

csviri Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm commented Jul 4, 2024

Uh oh!

metacosm commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

csviri Jul 4, 2024 •

edited

Loading