Move remaining controller code to structured logging#933
Move remaining controller code to structured logging#933google-prow-robot merged 3 commits intoknative:masterfrom mdemirhan:logging
Conversation
…y instead of glog and log.
google-prow-robot
added
the
size/L
| configuration.NewController(kubeClient, elaClient, buildClient, kubeInformerFactory, elaInformerFactory, cfg, *controllerConfig), | ||
| revision.NewController(kubeClient, elaClient, kubeInformerFactory, elaInformerFactory, buildInformerFactory, cfg, &revControllerConfig), | ||
| configuration.NewController(kubeClient, elaClient, buildClient, kubeInformerFactory, elaInformerFactory, cfg, *controllerConfig, logger), | ||
| revision.NewController(kubeClient, elaClient, kubeInformerFactory, elaInformerFactory, buildInformerFactory, cfg, &revControllerConfig, logger), |
There was a problem hiding this comment.
Does it make sense to wrap logger into revControllerConfig? Same for Configuration and Route. No ConfigurationControllerConfig nor RouteControllerConfig exist yet.
There was a problem hiding this comment.
Looking at revControllerConfig, it seems like it is specifically targeted at user configuration from the config maps. Logger doesn't seem like a natural fit there. Also, adding it to the config might cause confusion within revision controller for example, because the logger is also contained in controller.Base. Let me know if this makes sense.
…luded for example)
|
/lgtm |
grantr
left a comment
grantr
left a comment
There was a problem hiding this comment.
Thanks @mdemirhan! Let's see if I can approve with just an approving review...
| if err != nil { | ||
| log.Printf("Failed to get metadata: %s", err) | ||
| panic("Failed to get metadata") | ||
| logger.Panic("Failed to get metadata", zap.Error(err)) |
There was a problem hiding this comment.
Just noting that we shouldn't be panicking here, but it's fine for this PR to carry over. I think #932 covers this.
|
Oh I can't actually approve this because I'm not an owner (yet). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, mdemirhan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-prow-robot
added
the
approved
|
/retest |
6 participants
Change remaining controller code to use zap structured logging library instead of glog and log.