cmd logger: Change all cmd modules to use the same logger #953
Conversation
|
/lgtm |
cmd/log/log.go
Outdated
|
|
||
| func Info(msg string, keysAndValues ...interface{}) { | ||
| log.Info(msg, keysAndValues...) | ||
| } |
There was a problem hiding this comment.
What's the reason log is private and the methods are re-exported from this package? Why not make log public and delete the Error and Info functions?
There was a problem hiding this comment.
I see two reasons for that:
- This way we can call the logger with
log.Info(...)instead oflog.Log.Info(...) - This will allow future modifications for the logger, without changing all the callers (change only inside the wrapper functions)
There was a problem hiding this comment.
I see two reasons for that:
- This way we can call the logger with
log.Info(...)instead oflog.Log.Info(...)
This can already be effectively achieved if Log is public. Users can specify a dot import alias:
import (
. "github.com/openshift/hypershift/cmd/log"
)
func foo() {
Log.Info(...)
}The user could also alias it to whatever package name they want.
- This will allow future modifications for the logger, without changing all the callers (change only inside the wrapper functions)
What you're describing sounds like a logging interface. There's already a history of competing logging interfaces for Go, and we're currently not using any of them. Instead, we're just linking directly to Zap. If we wanted to switch to an interface, I think we need design consensus from the team, and we would first probably want to investigate using https://github.com/go-logr/logr, which is the interface controller-runtime is using (and we're tightly coupled to controller-runtime). But part of the reason to link directly to Zap is to take advantage of its specific features instead of using a lowest common denominator interface, so there's definitely a discussion to be had.
There was a problem hiding this comment.
Ok, I will change it now
This will require to change all lines using the logger from log. to Log.
There was a problem hiding this comment.
The make verify doesn't allow dot imports:
cmd/bastion/aws/create.go:13:2: should not use dot imports (ST1001)
There was a problem hiding this comment.
The linter isn't always the final arbiter of style here, have to get more feedback from the rest of the team. Another option would be for users to declare a local logger, like:
import (
logger "github.com/openshift/hypershift/cmd/log"
)
var log = logger.LogBut I can't speak for everyone on the team. This is really subjective.
There was a problem hiding this comment.
I'm not a fan of underscore imports either, because the compiler can't detect when they become unused. Both using log.Log or declaring a package-level variable like in your comment above seems fine to me.
There was a problem hiding this comment.
Declaring a package-level variable is actually how it was before this PR
In each package defined log.go file and defined the package variable in it
Do you prefer to keep the code this way?
There was a problem hiding this comment.
Either that or changing the log calls to log.Log.${Method}, both are fine with me
There was a problem hiding this comment.
I changed the code in this PR call: log.Log.${Method}
In case you prefer to use the package-level variable we can keep the previous implementation and close this PR
cmd/log/log.go was added in PR openshift#832 Change all cmd modules to use this cmd logger This is done following to the discussion here: openshift#832 (comment)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, nirarg 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 |
openshift-ci
bot
added
the
approved
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@nirarg: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
6 participants
What this PR does / why we need it:
cmd/log/log.gowas added in PR #832Change all cmd modules to use this cmd logger
This is done following to the discussion here:
#832 (comment)
Checklist