-
Notifications
You must be signed in to change notification settings - Fork 508
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
⚠️ Create a dedicated logging package to encapsulate calls to zap
#1502
Conversation
Integration tests success for |
Integration tests success for |
Creates a wrapper around existing `zap.Logger` to make it easier to replace/extend with scorecard logging. Signed-off-by: Stephen Augustus <[email protected]>
c36a736
to
37d746b
Compare
Integration tests success for |
Signed-off-by: Stephen Augustus <[email protected]>
Signed-off-by: Stephen Augustus <[email protected]>
Signed-off-by: Stephen Augustus <[email protected]>
zap
37d746b
to
4b356a6
Compare
@inferno-chromium @naveensrinivasan @azeemshaikh38 @laurentsimon @olivekl @jeffmendoza -- This is in a good place for initial review. I don't want run to far ahead in case there's something design-wise that needs fixing. |
Integration tests success for |
I had a quick look and 👍. I will let @azeemshaikh38 and @laurentsimon chime in |
Thanks so much, Naveen! |
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.
This looks awesome @justaugustus, thanks!
Signed-off-by: Stephen Augustus <[email protected]>
Signed-off-by: Stephen Augustus <[email protected]>
4b356a6
to
ac1d50f
Compare
@naveensrinivasan @azeemshaikh38 -- Fixed up the reviews. Should be good to retrigger CI on :) |
Thanks! |
Integration tests success for |
Codecov Report
@@ Coverage Diff @@
## main #1502 +/- ##
==========================================
+ Coverage 50.42% 50.43% +0.01%
==========================================
Files 68 68
Lines 6071 6069 -2
==========================================
Hits 3061 3061
+ Misses 2809 2807 -2
Partials 201 201 |
Thanks @justaugustus, looks good to merge to me. |
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.
Just wanted to join and say thanks! This is a massive PR!
Please check if the PR fulfills these requirements
Tests for the changes have been added (for bug fixes / features)
We're roughly mimicking existing functionality and the interface may change in future. Logging usage that has changed exists in packages that already have test coverage.
tl;dr -- If existing tests pass, we should be fine here.
PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Introduces a logging package to eventually replace go.uber.org/zap.
Here we're effectively quarantining usage of the go.uber.org/zap and go.uber.org/zap/zapcore to a single package,
log
.What is the current behavior? (You can also link to an open issue here)
See Should scorecard be forcing users to use a zap.Logger? #1273.
What is the new behavior (if this is a feature change)?
Functions that create instances of
zap.Logger
now create instances oflog.Logger
:...which means usage of zap will now look something like:
As @jeffmendoza mentions in Should scorecard be forcing users to use a zap.Logger? #1273 (comment):
the goal would to support logging without zap, opting to use
DetailLogger
instead.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Consumers of functions like:
will need to use
log.Logger
as an argument:as well as update log calls from:
to:
Other information: