-
Notifications
You must be signed in to change notification settings - Fork 315
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
Add -log-level flag to inject-connect, healthchecks debug logs #400
Conversation
8eb103d
to
14f0384
Compare
Level: hclog.Info, | ||
Output: os.Stderr, | ||
}) | ||
logger := hclog.Default() |
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 is equivalent.
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.
Should we use the constructor here?
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.
You mean swap it back to hclog.New() or use `common.Logger("info")?
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.
use common.Logger(hclog.Info)
for consistency?
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.
Not a huge deal for me one way or the other btw.
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.
Good idea because we might add more to that function and expect it to apply to all 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.
looks great!
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.
Looks good! Left a couple of comments, but they are not blocking.
Level: hclog.Info, | ||
Output: os.Stderr, | ||
}) | ||
logger := hclog.Default() |
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.
Should we use the constructor here?
Also standardize on how we construct logger throughout commands.
Changes proposed in this PR:
-log-level
flag toinject-connect
so we can configure the health check log levelcommon
so it can be reusedHow I've tested this PR:
How I expect reviewers to test this PR:
Checklist: