-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix: #161: replace prints with logs #772
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
builderID = outBuilderID | ||
fmt.Fprintf(os.Stderr, "Verifying npm package %s: PASSED\n\n", tarball) |
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.
where does slog write the result? We need to ensure compatibility and ensure it's written to stderr when fmt.Fprintf(os.Stderr
. Otherwise folks won't be able to pipe the result into jq or other tools: jq will pick not just the printed provenance, but the other messages too.
I think before changing this API we should define what the interface is for our logger. And we should provide an interface as logger, so that callers can customize how they log. Here's a fast logging library https://github.com/uber-go/zap, among others, that would need to be instantiable by callers
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.
I confirmed that by default slog will send to stderr. And then I removed the changes against fmt.Fprintf(os.Stdout, ...
statements.
I'll look into how to best allow caller-provided loggers.
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.
FWIW, I'm personally in favor of using the stdlib logging packages rather than including a dependency.
Signed-off-by: Ramon Petgrave <[email protected]>
784ebdb
to
ff20e18
Compare
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
ff20e18
to
af32d96
Compare
Addresses #161, and followup to #768
This PR replaces all
fmt
print statements withlog/slog
logs at appropriate levels.for successes
and for failures
If you're using slsa-verifier as a library, you can suppress INFO logs by invoking