Add helm variables to choose controllers of karmada and kubernetes ma… #2542
Conversation
karmada-bot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @fredgate! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/S
fredgate
force-pushed
the
feature/helm-value-controllers
branch
from
5c19b5a to
2c0e322
Compare
…nager Signed-off-by: fredgate <barriere.fr@gmail.com>
fredgate
force-pushed
the
feature/helm-value-controllers
branch
from
2c0e322 to
55744e0
Compare
|
/lgtm |
| @@ -261,6 +261,9 @@ controllerManager: | |||
| ## - myRegistryKeySecretName | |||
| ## | |||
| pullSecrets: [] | |||
| ## @param controllerManager.config.controllers the controllers launched by the manager. | |||
| config: | |||
There was a problem hiding this comment.
I prefer to use controllers directly. config looks a bit redundant.
There was a problem hiding this comment.
I can use controllers directly instead of config.controllers, but I preferred the latter because it will be more organized if some additional parameters need to be added such as cluster-status-update-frequency or v...
All these values will be clearly under controllerManager.config rather than directly at the root of controllerManager, which I find more readable.
What do you think about it ?
There was a problem hiding this comment.
I got what you mean. I prefer to put some related configuration items under the same group. It's ok to use controllers directly when there are currently no other options.
BTW, I don't think config is a good group name. All of them in controllerManager belong to the category of config.
## controller manager config
controllerManager:
There was a problem hiding this comment.
I can use controllers directly instead of config.controllers, but I preferred the latter because it will be more organized if some additional parameters need to be added such as cluster-status-update-frequency or v...
I agree that we may need to customize other parameters later, they are organized together would be great!
I don't think config is a good group name. All of them in controllerManager belong to the category of config.
How about args?
There was a problem hiding this comment.
Yes args seems like a better name to me.
Should we use it as
- an object with predefined properties (like I did in the PR)
- an array where as many elements can be added freely. We could fill this variable like this during a release
controllerManager:
args:
- --controllers=-namespace,*
- --v=3
The latter makes it possible to better cover all the possibilities without modification.
There was a problem hiding this comment.
I think the former is better. The latter may confuse users about which configuration items can be configured.
There was a problem hiding this comment.
The latter option allows to configure all args of karmada-controller-manager, that allows the user to configure anything he wants, not just what we imagine : there can always be unimagined scenarios, like for example mine. And both current and future. And the chart will be compatible without change not only with current arguments but also with future ones.
@RainbowMango What do you think about this two solutions ?
There was a problem hiding this comment.
The latter option allows to configure all args of karmada-controller-manager, that allows the user to configure anything he wants, not just what we imagine
The idea is great, but my concern is the user experience. The users might need to specify all args at here, even they just want to customize the --controllers.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allow to choose controllers executed by karmada and kubernetes manager of control plane when deploying with the Helm chart.
Which issue(s) this PR fixes:
Fixes #2512
Special notes for your reviewer:
Does this PR introduce a user-facing change?: