✨ Add options to configure a logger for manager and controllers #1095
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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
approved
vincepri
force-pushed
the
logger-manager-controller
branch
from
64221d9 to
c8755c5
Compare
pkg/builder/controller.go
Outdated
| if ctrlOptions.Log == nil { | ||
| ctrlOptions.Log = blder.mgr.GetLogger() | ||
| } | ||
| ctrlOptions.Log = ctrlOptions.Log.WithValues("groupVersionKind", gvk.String()) |
There was a problem hiding this comment.
How do we properly distinguish this from what we do in the encoder
?I think the log key should maybe be reoncilerGroupVersionKind or something like that?
There was a problem hiding this comment.
It seems like the logging here should remove the need to enrich the log entries in the zap helper. Would you expect any delta between the gvk defined here and the resource passed to reconcile?
There was a problem hiding this comment.
Its totally possible that during the reconciliation of an object type a an object type b has to be updated and that fails so I think the two are complementary.
There was a problem hiding this comment.
Fixed, used reconcilerGroup and reconcilerKind
pkg/builder/controller.go
Outdated
| if ctrlOptions.Log == nil { | ||
| ctrlOptions.Log = blder.mgr.GetLogger() | ||
| } | ||
| ctrlOptions.Log = ctrlOptions.Log.WithValues("groupVersionKind", gvk.String()) |
There was a problem hiding this comment.
It seems like the logging here should remove the need to enrich the log entries in the zap helper. Would you expect any delta between the gvk defined here and the resource passed to reconcile?
Signed-off-by: Vince Prignano <[email protected]>
vincepri
force-pushed
the
logger-manager-controller
branch
from
c8755c5 to
829c816
Compare
|
@vincepri: 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/test-infra repository. I understand the commands that are listed here. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Signed-off-by: Vince Prignano [email protected]
/assign @alvaroaleman @detiber
Made these changes also backward compatible to be able to go in v0.6.2