馃悰 Inject manager's logger instead of internal one #1289
Conversation
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @timebertt. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
| @@ -251,7 +251,7 @@ func (cm *controllerManager) SetFields(i interface{}) error { | |||
| if _, err := inject.MapperInto(cm.mapper, i); err != nil { | |||
| return err | |||
| } | |||
| if _, err := inject.LoggerInto(log, i); err != nil { | |||
| if _, err := inject.LoggerInto(cm.logger, i); err != nil { | |||
There was a problem hiding this comment.
What a mess this is, so the log variable is:
logf.RuntimeLog.WithName("manager")where
->logf.RuntimeLogislog.Log.WithName("controller-runtime")where
->log.LogisNewDelegatingLogger(NullLogger{})
And the managers logger is defaulted to logf.Log
So I think we should have the manager default options.Logger to what is currently the log variable instead, right?
ALso, can you check if the log variable is used by anything else and if not, remove it and move the construction of its logger into the managers log defaulting?
/ok-to-test
There was a problem hiding this comment.
Yes, valid point.
Changed the defaulting of the manager's logger to logf.RuntimeLog.WithName("manager") and switched the remaining usages of the log variable now.
Hope I got you right, PTAL
k8s-ci-robot
added
ok-to-test
timebertt
force-pushed
the
fix/inject-manager-logger
branch
from
f782be1
to
b9197ef
Compare
k8s-ci-robot
added
size/S
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, timebertt 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 |
k8s-ci-robot
added
the
approved
Before this PR, the manager injected the controller-runtime-internal logger into controllers, predicates and so on, that implemented the
inject.Loggerinterface. This means, controllers and so on were provided with a logger which name is set tocontroller-runtime.manager.I found this behaviour to be confusing, especially if you provided the manager with a custom logger via
manager.Options.Logger(e.g. with a different top-level name).With this PR, the manager injects its own logger or the default one, if none is specified.