-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Union sock options #2047
Union sock options #2047
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2047 +/- ##
==========================================
- Coverage 93.9% 92.69% -1.22%
==========================================
Files 34 29 -5
Lines 1280 1163 -117
Branches 368 338 -30
==========================================
- Hits 1202 1078 -124
- Misses 71 81 +10
+ Partials 7 4 -3
Continue to review full report at Codecov.
|
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'm -1 on this change. And this is breaking changes, so we cannot merge to the master branch.
@EslamHiko If you want to change something(e.g. add new feature, breaking changes, etc...), please submit an issue because we need to discuss before implementation. Thanks. |
Yes, we should do this for next branch, also we still need discussion how handle CLI arguments for this, i.e. we need e2e tests for this |
@evilebottnawi I'll change the base branch to next, and for CLI arguments I handled them in lib/utils/createConfig.js |
dd9618b
to
a2a5102
Compare
|
This reverts commit fe80fa4.
@hiroppy @evilebottnawi I'm Sorry, I accidentally merged this PR into next branch. I'll revert it and create another PR. |
For Bugs and Features; did you add new tests?
Used existing tests
Motivation / Use-Case
It's in the To Do list
Breaking Changes
Using sockHost, sockPath and sockPort options will be invalid. they must be used in the new clientSocketOptions option.
Additional Info