-
Notifications
You must be signed in to change notification settings - Fork 39.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Federation][Kubefed] Bug fix to enable disabling federation controllers through override args #44209
Conversation
@k8s-bot cvm gce e2e test this |
I'm not super enthused that this could break people's scripted usages of kubefed, since it's now in beta. That said, I'm not sure that I can see a better way to do this, except revamping the whole system to use some sort of configuration file rather than command-line parameters. Which, incidentally, is how kubeadm does this: https://kubernetes.io/docs/admin/kubeadm/#config-file I'm wondering if we should move from a flag to doing this the way kubeadm does. Of course, this is complicated by the fact that kubefed is in beta now. |
Is this potential complexity worth it? Or might it be preferable to expose any multivariate configuration option for apiserver or controllermanager as a first-class argument to |
Yes. We have discussed this before. Someone should bite the bullet. But note that we want to go the ComponentConfig route. SIG-node and SIG-scheduling are working on that. So it might be worth discussing with them. I had a chat with @mtaufen long back. I don't know the current status though.
How is that less complex? |
Should I limit this PR to only update the marshaling of arguments such that |
Yeah, I think we should do that to begin with and cherry-pick that to v1.6.x branch before we resolve the other issues with syntax changes for 1.7 |
…ers through override args
956d84c
to
fe87957
Compare
updated, PTAL! |
More complex in terms of UX, not implementation. I think it would be simpler for the user to provide a first-class argument to |
There are pros and cons. There are the advantages that you mention here. But on the other hand
|
What's here LGTM, for the issue with marshaling multiple |
Agreed. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irfanurrehman, madhusudancs
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@irfanurrehman please also cherry-pick this to |
Automatic merge from submit-queue (batch tested with PRs 44286, 44209) |
Automatic merge from submit-queue [Federation][Kubefed] Bug fix to enable disabling federation controllers through override args Targets #42761 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # #42761 **Special notes for your reviewer**: PR #44209 is merged to master. This fix needs to be cherry-picked to release-1.6 also. **Release note**: ```release-note Fix for [disabling federation controllers through override args](#42761). ```
…lease-1.6 Automatic merge from submit-queue [Federation][Kubefed] Bug fix to enable disabling federation controllers through override args Targets kubernetes#42761 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # kubernetes#42761 **Special notes for your reviewer**: PR kubernetes#44209 is merged to master. This fix needs to be cherry-picked to release-1.6 also. **Release note**: ```release-note Fix for [disabling federation controllers through override args](kubernetes#42761). ```
Targets #42761
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes ##42761
Special notes for your reviewer:
@marun @perotinus @nikhiljindal
Release note: