-
Notifications
You must be signed in to change notification settings - Fork 702
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
Replace logrus
by klog
#4180
Replace logrus
by klog
#4180
Conversation
@@ -518,7 +518,7 @@ func pullAndExtract(repoURL *url.URL, appName, tag string, puller helmutils.Char | |||
|
|||
func chartImportWorker(repoURL *url.URL, r *OCIRegistry, chartJobs <-chan pullChartJob, resultChan chan pullChartResult) { | |||
for j := range chartJobs { | |||
log.WithFields(log.Fields{"name": j.AppName, "tag": j.Tag}).Debug("pulling chart") | |||
log.V(4).Infof("pulling chart, name=%s, tag=%s", j.AppName, j.Tag) |
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.
Why not use klog's support for structured logging here and above, with something like:
log.V(4).Infof("pulling chart, name=%s, tag=%s", j.AppName, j.Tag) | |
log.V(4).InfoS("pulling chart", "name", j.AppName, "tag", j.Tag) |
or even
log.V(4).Infof("pulling chart, name=%s, tag=%s", j.AppName, j.Tag) | |
log.V(4).InfoS("pulling chart", "chartJob", j) |
Better eg showing the difference: https://go.dev/play/p/EW5ZQh7MDXI
Or maybe you want to do that separately so it's consistent across the codebase (personally I'd think it OK to do it as we update, like here, but up to you).
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.
Thanks for the suggestion and the playground example. I haven't given it a try yet. Really interesting.
As per #3848 (comment), I already have planned to do so in a separate PR once this stack of PRs became merged. I've added a card to our board not to forget about it. I've created an issue.
9f58bce
to
1c20257
Compare
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
c3f2ed6
to
0b12a08
Compare
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: cmd/apprepository-controller/cmd/root.go cmd/asset-syncer/cmd/root.go cmd/asset-syncer/server/utils.go cmd/assetsvc/cmd/root.go cmd/kubeapps-apis/cmd/root.go pkg/agent/docker_secrets_postrenderer.go
Description of the change
This PR continues the effort started some time ago in unifying the logging libraries. We opted for
klog
, the one used in Kubernetes. Some minor changes have been introduced in order to adapt the log operations to the ones available in klog (noWithParams
orWithError
are available here).Benefits
Reduce same-purpose deps. It also simplifies the task of moving to structured logging as we were asked in the past.
Possible drawbacks
N/A
Applicable issues
Additional information
N/A