⚠️ NewTypedUnmanaged: Stop requiring a manager #3141
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/L
| } | ||
|
|
||
| // DefaultFromConfig defaults the config from a config.Controller | ||
| func (options *TypedOptions[request]) DefaultFromConfig(config config.Controller) { |
There was a problem hiding this comment.
What is notably absent from this change are tests - That is because this behavior is already verified by existing tests
alvaroaleman
changed the title
alvaroaleman
force-pushed
the
factor-out
branch
2 times, most recently
from
df0838a to
3b05ebd
Compare
alvaroaleman
force-pushed
the
factor-out
branch
2 times, most recently
from
deb06d8 to
ceb6d37
Compare
Currently, `NewUnmanaged` and `NewTypedUnmanaged` require a manager but the only properties they ever access from it are `GetControllerOptions` and `GetLogger`. It makes no sense to require a `Manager` for something that is unmanaged, so remove that. Doing so naively means that we require two options parameters for these constructors, one that has the actual options and another one to default from which again is confusing. Remove the options struct to default from and instead implement a `DefaultFromConfig` that defaults the controller options `config.Controller` which also clarifies what belongs where. This defaulting gets automatically called in the managed constructors and is exported so anyone can use it. This change is breaking only for users of `New[Typed]Unmanaged` in that: * The signature changes * The defaulting of the `controller.Options` from `config.Controller` doesn't happen anymore, but it is still possible to do that explicitly
|
@alvaroaleman: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thank you! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 607fe96b999392c4a720f02a48a0fc6d8debe0ec
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reference: kubernetes-sigs/controller-runtime#3141 On-behalf-of: SAP <[email protected]> Signed-off-by: Marvin Beckers <[email protected]>
Currently,
NewUnmanagedandNewTypedUnmanagedrequire a manager but the only properties they ever access from it areGetControllerOptionsandGetLogger. It makes no sense to require aManagerfor something that is unmanaged, so remove that.Doing so naively means that we require two options parameters for these constructors, one that has the actual options and another one to default from which again is confusing. Remove the options struct to default from and instead implement a
DefaultFromConfigthat defaults the controller optionsconfig.Controllerwhich also clarifies what belongs where. This defaulting gets automatically called in the managed constructors and is exported so anyone can use it.This change is breaking only for users of
New[Typed]Unmanagedin that:controller.Optionsfromconfig.Controllerdoesn't happen anymore, but it is still possible to do that explicitly