-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(pcr0tool): Add support of a logger #347
Conversation
pkg/log/fiano_logger.go
Outdated
|
||
var _ fianoLog.Logger = FianoLogger{} | ||
|
||
// FianoLogger is just a placeholder for a logger, which does nothing |
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 isn't true? We've gone from it doing nothing to being a wrapper around a logger. Also should the methods below not be checking if Backend is nil?
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.
Yeah, replaced DummyLogger
with FianoLogger
, but forgot to update the comments. Fixed.
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.
What about the nil checks? You do fianoLog.DefaultLogger = log.FianoLogger{}
in various places, which I think means if Fiano tries to log it's going to try to call a function on Backend which will be nil?
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.
IDE replaced it everywhere (I used the smart renaming function of IDE) and I just didn't know about these use cases in the code :(
Thanks for noticing! Fixed.
I just made FianoLogger
safe using:
if l.Backend == nil {
return
}
pkg/log/fiano_logger.go
Outdated
// FianoLogger is just a placeholder for a logger, which does nothing | ||
type FianoLogger struct { | ||
Backend logger.Logger | ||
LogAsLevel logger.Level |
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.
Do these need to be exported? I don't see them used outside this package?
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.
Any reason to hide it? :)
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 generally lean towards not exporting things that aren't needed so interface boundaries are kept to only the required pieces, but no strong push towards that here given it's all internal.
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.
It seems to me C++-y approach. I'd prefer in Go to export by default if there are no explicit safety concerns. In my opinion in Go we need to protect fields only if there are hidden assumptions about these fields, which does not seem the case here. But this is not a strong opinion, feel free to insist.
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 insisting here, but let's have a conversation about it when I'm back after PTO.
3eb3375
to
5660f02
Compare
77e09bc
to
73796eb
Compare
2fefce6
to
e29df8b
Compare
I know this took way too long on my side, sorry for that. Can you rebase and fix, push and i will happily review and merge this week. |
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. Can be merged when rebased :)
e29df8b
to
944380a
Compare
944380a
to
92e419a
Compare
The PR is beyond recovery by now.
No worries. A lot of stuff changes since then, and logger was basically introduced as a side effect of other PRs anyway. |
Adding basic support of a Logger to
pcr0tool
.Test Plan
Ran:
Output:
The first line is:
(as expected)