-
Notifications
You must be signed in to change notification settings - Fork 2
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
Introduce server config method(s) #76
Introduce server config method(s) #76
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 91.31% 91.58% +0.27%
==========================================
Files 20 21 +1
Lines 829 844 +15
==========================================
+ Hits 757 773 +16
+ Misses 53 52 -1
Partials 19 19 ☔ View full report in Codecov by Sentry. |
@ksysoev sorry for the delay. This PR is ready for review. I have changed a few things
|
server/server.go
Outdated
@@ -69,7 +64,7 @@ func WithReadinessChan(ch chan<- struct{}) Option { | |||
// NewServer creates new instance of Wasabi server | |||
// port - port to listen on | |||
// returns new instance of Server | |||
func NewServer(addr string, opts ...Option) *Server { | |||
func NewServer(addr string, serverConfig ServerConfig, opts ...Option) *Server { |
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.
Let's not introduce new mandatory arguments, I think in majority of cases these timeouts don't need to be redefined and reasonable default can be use without bothering the user. I'd suggest to pass it as optional parameter.
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.
added an optional method
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, Thanks for the contribution
Closes #68