Simplify controller registration and unify controller configuration #303
Conversation
| @@ -24,7 +24,7 @@ public static void main(String[] args) throws IOException { | |||
| Config config = new ConfigBuilder().withNamespace(null).build(); | |||
| KubernetesClient client = new DefaultKubernetesClient(config); | |||
| Operator operator = new Operator(client, DefaultConfigurationService.instance()); | |||
| operator.registerControllerForAllNamespaces(new SchemaController(client)); | |||
| operator.register(new SchemaController(client)); | |||
There was a problem hiding this comment.
How does user can configure the namespaces in runtime then?
Note that whatever is going to be implemented should work for pure-java operators as well.
There was a problem hiding this comment.
This is done by using namespaces = ControllerConfiguration.WATCH_ALL_NAMESPACES_MARKER on the Controller annotation.
There was a problem hiding this comment.
That’s not changale in runtime,
In pure java operators it still should be possible for user to get the namespaces some how and respct them in registration.
There was a problem hiding this comment.
This should be overridden via externalized configuration.
There was a problem hiding this comment.
Not sure what external configuration for pure-java applications mean!
Isn’t that too opinionated? Also do we have this possibility already?
I’m asking because I’m using the removed methods to configure my controllers already and there may be other users who use them, so we need to offer a reasonable alternative before removing them
There was a problem hiding this comment.
User 1 wants to run operatopr OP1 just for namespace ns1
User 2 wants to run operatopr OP1 for namespaces ns1 and ns2
User 3 wants to run operatopr OP1 for all the namespaces
The operator is in pure-java and is deciding which namespaces are in the interest of the operator via command-line arguments.
So it either calls registerControllerForAllNamespaces method if ALL is sent as argument method or call registerController with the list of given namespaces.
Now the question is how the same thing can be achieved after removing these methods?
There was a problem hiding this comment.
That makes sense. How about a register(controller, overrideConfiguration) method? It's probably more complex to implement than keeping the separate methods but the issue is that we should always go through the configuration to register controllers because that also has impacts on the event sources and it's not done consistently everywhere.
There was a problem hiding this comment.
Yep it makes sense to have a configuration class with convenient to use method to let the user change the stuff that are useful to be configured in runtime like: retry, namespaces, cluster scoped,...
There was a problem hiding this comment.
Ok, I'll get on that then! 😄
There was a problem hiding this comment.
This is now done.
metacosm
force-pushed
the
clean-register
branch
from
42f5efd to
fed51b0
Compare
metacosm
force-pushed
the
clean-register
branch
3 times, most recently
from
dad1b33 to
5921a6c
Compare
The intent here is that controllers should be registered according to their configuration. This will become even more relevant when externalized configuration is in place (see #237). This also means using ControllerConfiguration.WATCH_ALL_NAMESPACES_MARKER in the namespaces field of the Controller annotation where appropriate since that's what ControllerConfiguration uses to determine if a controller should watch all namespaces. This still needs to be unified across the code base (see #302).
metacosm
force-pushed
the
clean-register
branch
from
5921a6c to
456ea3c
Compare
|
+100 to get this merged. I have built this locally and tried it. |
|
Thanks @wtrocki ! let's merge it then |
Fixes #302