-
Notifications
You must be signed in to change notification settings - Fork 39k
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
complete the controller context for init funcs #46783
complete the controller context for init funcs #46783
Conversation
@@ -274,11 +281,6 @@ func KnownControllers() []string { | |||
|
|||
ret.Insert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the comment about special controllers here, and give guidance if people should not generally add anything else to this list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
5056b80
to
fd38632
Compare
fd38632
to
07fbd4a
Compare
|
||
sharedInformers.Start(stop) | ||
// start the first set of informers now so that other controllers can start | ||
ctx.InformerFactory.Start(ctx.Stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it odd to tie sharedInformers to this controller? if you don't provide a private key and this never starts, doesn't it block other controllers? if you're not running with --use-service-account-credentials
, what else in the controller manager depends on this controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it odd to tie sharedInformers to this controller?
Not really, this is the logical order from before, it's just slightly more obvious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what else in the controller manager depends on this controller?
Not completely certain. I think the future is subdivision.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
07fbd4a
to
d4d57b8
Compare
d4d57b8
to
475916c
Compare
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
Queue is confused. Etcd3 passed |
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
1 similar comment
|
Automatic merge from submit-queue (batch tested with PRs 40760, 46706, 46783, 46742, 46751) |
This completes the conversion to initFuncs for the controller initialization to make easier and more manageable to add them.