-
Notifications
You must be signed in to change notification settings - Fork 343
Add client config param IgnoreTURNResolveErrors #455
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #455 +/- ##
==========================================
+ Coverage 67.43% 67.57% +0.13%
==========================================
Files 43 43
Lines 3083 3087 +4
==========================================
+ Hits 2079 2086 +7
+ Misses 843 841 -2
+ Partials 161 160 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
20a5223
to
a170ed1
Compare
d94f5e4
to
752326c
Compare
@@ -47,6 +47,8 @@ type ClientConfig struct { | |||
Conn net.PacketConn // Listening socket (net.PacketConn) | |||
Net transport.Net | |||
LoggerFactory logging.LoggerFactory | |||
|
|||
IgnoreTURNResolveErrors bool // TURN server address is not required for some configurations (e.g. proxy) |
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 don't think we should add a config, there are many reason, but it feels like a single use flag.
What if we make a new constructor NewClientWithResolver
and essentially make the nested condition after if len(config.TURNServerAddr) > 0 {
customizable, we can then make the current NewClient
call this new constructor with the old resolver logic.
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.
Even better, we can use Pion options, pattern, and make NewClientWithOptions
and make a custom option WithResolver
.
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 can make a PR for this!
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.
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.
@JoeTurki Any updates on this?
Description
In some configurations we don't use the TURN address at the client side (e.g. proxy connection, see #450). But now at the client setup the provided turn address must be resolvable.
Adding client config param
IgnoreTURNResolveErrors
that allows omit resolving errors when we don't need turn address.Also discussed here: pion/ice#773