-
Notifications
You must be signed in to change notification settings - Fork 31
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
Make the logs quieter #274
Conversation
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.
LGTM other than minor spelling errors and (possibly?) the error level for an authorization failure.
@@ -76,14 +76,14 @@ real happens.`, | |||
|
|||
verifyModules := viper.GetBool("verify-modules") | |||
if !verifyModules { | |||
clog.Warn("skipping module verfiction") | |||
clog.Warn("skipping module verfication") |
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.
did you mean: "verification"?
@@ -53,12 +53,12 @@ not display healthy checks.`, | |||
|
|||
verifyModules := viper.GetBool("verify-modules") | |||
if !verifyModules { | |||
log.WithField("component", "client").Warn("skipping module verfiction") | |||
log.WithField("component", "client").Warn("skipping module verfiaction") |
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.
"verification"
@@ -106,7 +106,7 @@ func (e *executor) Plan(in *pb.LoadRequest, stream pb.Executor_PlanServer) error | |||
logger = logger.WithField("function", "executor.Plan") | |||
|
|||
if err := e.auth.authorize(ctx); err != nil { | |||
logger.WithError(err).Info("authorization failed") | |||
logger.WithError(err).Warning("authorization failed") |
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.
Is an authorization failure a Warning
or an Error
?
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.
On the server, it's a warning since a user failing authentication should be raised for audit purposes but it's a normal occurrence. It's (still) an error on the client side.
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.
Ah, that makes sense.
Made the spelling fixes, thanks for the catch @rebeccaskinner. I'll rebase after your next review :) |
5a025e5
to
d66cfa1
Compare
The lint errors here are due to sample output, which we maybe shouldn't change. They're in code blocks anyway, so I'm not sure what it's complaining about. |
Fixes #248