-
Notifications
You must be signed in to change notification settings - Fork 4
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
Code cleanup and refactoring #43
Conversation
ywwg
commented
Apr 5, 2022
•
edited
edited
- Add support for handling SIGTERM signal
- Refactor Run function into api module instead of binary so it can be called from e2e tests without needing to start up the binary itself.
- Add wrappers for error and warning logging so we don't need to keep ignoring the return value
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
I think the sigterm handling needs to change a bit if we use the pkg/server instead of the weaveworks/common server, since pkg/server doesn't have its own SignalHandler interface |
ok I'll update after your change goes in |
This comment has been minimized.
This comment has been minimized.
hand-tested on the command line, works |
This comment has been minimized.
This comment has been minimized.
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 like the new logging layer!
// Error logs an error and panics if the log call itself had an error. | ||
func Error(logger log.Logger, keyvals ...interface{}) { | ||
if err := level.Error(logger).Log(keyvals); err != nil { | ||
panic(fmt.Sprintf("error writing to log: %v", err)) |
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 curious - why %v
and not %w
?
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.
oh yeah I can do that
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.
actually I can't! %w is only supported in fmt.Errorf
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.
Ohh gotcha, interesting
oh hm %w doesn't work in sprintf |
looks like I broke tests so I'll do that after lunch |
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
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!