Thanks for the kind reply.
Should I change this name, but still keep things like the realm/socket as
> members still? I think of turn.Server as the global singleton that handles
> all traffic (UDP or TCP)
An interface is used to allow varying types to have the same logic applied.
In this case when you assign a Server interface var the only use will be to
check client authentication. Sometimes people use type assertions on
interfaces but my opinion is that shouldn’t be a regular thing. The use of
the interface var vs how it’s done are decoupled.
I am only at 7% coverage, I am working on adding more. It isn't feature
> complete, I have been more worried about getting users (and getting
> valuable testing eyes) I will add more tests this weekend.
Unit tests are one part, but I’d be more interested in some sort of overall
functionality test maybe with load characteristics that perhaps benchmarks.
If you can show the example works then that may help attract users.
Does that thinking all seem sane?
Well I don’t think a couple extra directories under examples and a few
extra tens or hundreds of KB will make much difference. Any dependencies
like Redis would only be needed if building the example. If necessary a
vendorer could remove files they don’t want.
I use the pkg repo to export things that people might actually want (like
> generic packet handling) and will be working on another project that uses
> it (native WebRTC client in Golang)
I saw the STUN things there which seems to match what I was describing.
Matt
On Saturday, May 19, 2018 at 2:07:38 AM UTC-5, se...@pion.ly wrote:
>
> This review is fantastic! I committed what I understood, I have a few
> questions though.
>
> > Instead of turn.Server a more idiomatic interface name could be
> something like turn.ClientAuthenticator
> Should I change this name, but still keep things like the realm/socket as
> members still? I think of turn.Server as the global singleton that handles
> all traffic (UDP or TCP)
>
> > package server doesn’t have tests and neither does package turn. How did
> you validate it? Is there validation code you can include?
> I am only at 7% coverage, I am working on adding more. It isn't feature
> complete, I have been more worried about getting users (and getting
> valuable testing eyes) I will add more tests this weekend.
>
> > Instead of cmd/simple-turn perhaps you could have
> examples/simple/main.go. Having a cmd indicates that the cmd is supported,
> but the authentication is minimal in simple-turn and might not be good for
> some uses.
>
> The nice thing about simple-turn is for those easy use cases (I want a
> TURN server that just has 5 users)
> I was thinking of making another repo, maybe turn-examples? That I could
> make a bunch of examples like Redis integration a REST server etc... I
> don't want to put that code in this repo because if it is vendored it will
> cause bloat for people.
> Does that thinking all seem sane?
>
> > Standards like STUN and TURN are an opportunity to make portable code.
> This layout might lead to clearer and more reusable code:
> Would it be ok if I kept the current layout, but created a `turn-examples`
> repo? I am mostly worried about bloating code bases that try to vendor
> pion-turn
>
> I use the pkg repo to export things that people might actually want (like
> generic packet handling) and will be working on another project that uses
> it (native WebRTC client in Golang)
>
>
> Thank you so much for the review again! I really appreciate the
> improvements you have provided.
>
> On Tuesday, May 15, 2018 at 7:46:46 AM UTC-7, matthe...@gmail.com wrote:
>>
>> Hello, thanks for sharing here and thanks for the MIT license. Here’s a
>> code review.
>>
>> These are my unfiltered opinions. You may not agree with some or any of
>> them, and some or all of them might not be reasonable to implement. My goal
>> is to build an ideal way to write Go code by doing code reviews here. My
>> hope is this feedback is useful to you in this project or in a future
>> project.
>>
>> In cmd/simple-turn “type myTurnServer struct {}” is a code smell that
>> hints at overusing interface, but in this case including usersMap in type
>> myTurnServer would remove the smell. Having the application do
>> authentication makes sense.
>>
>> (application meaning code using the library)
>>
>> Instead of turn.Server a more idiomatic interface name could be something
>> like turn.ClientAuthenticator.
>>
>> package server doesn’t have tests and neither does package turn. How did
>> you validate it? Is there validation code you can include?
>>
>> Perhaps consider embedding Protocol in allocation.FiveTuple.
>>
>> Instead of cmd/simple-turn perhaps you could have
>> examples/simple/main.go. Having a cmd indicates that the cmd is supported,
>> but the authentication is minimal in simple-turn and might not be good for
>> some uses.
>>
>> Standards like STUN and TURN are an opportunity to make portable code.
>> This layout might lead to clearer and more reusable code:
>>
>> github.com/pions/turnhost
>> library code files that simplify making a TURN host
>> /examples
>> /simple
>> main.go
>> /turn
>> library code files implementing the symbols and logic of the
>> standards
>>
>> Can you explain the github.com/pions/pkg/stun pattern? There is no pkg
>> directory included in the base repo.
>>
>> Thanks,
>> Matt
>>
>> On Monday, May 14, 2018 at 4:35:09 PM UTC-5, se...@pion.ly wrote:
>>>
>>> Hi list!
>>>
>>> I wrote a TURN server and would love to get feedback/share
>>> https://github.com/pions/turn
>>>
>>>
>>> If you aren't interested in the code, but just want a TURN server there
>>> are already built releases that work on Windows/Darwin/Linux/FreeBSD and
>>> should just take 5 mins to get running!
>>> These are the goals I had in mind when designing it, I was frustrated
>>> with other solutions and feel like it creates a higher barrier of entry to
>>> building WebRTC products then needed.
>>>
>>>
>>> # Easy Setup
>>>
>>> The example cmd (simple-turn) is a statically built TURN server, configured
>>> by environment variables.
>>>
>>> The entire install setup is 5 commands, on any platform! The goal is that
>>> anyone should be able to run a TURN server on any platform.
>>>
>>> # Integration first
>>> pion-turn makes no assumptions about how you authenticate users, how you
>>> log, or even your topology! Instead of running a dedicated TURN server you
>>> can inherit from github.com/pions/turn and set whatever logger you want.
>>>
>>> # Embeddable
>>> You can add this to an existing service. This means all your config files
>>> stay homogeneous instead of having the mismatch that makes it harder to
>>> manage your services.
>>> For small setups it is usually an overkill to deploy dedicated TURN
>>> servers, this makes it easier to solve the problems you care about.
>>>
>>> ## Readable
>>> All network interaction is commented with a link to the spec. This makes
>>> learning and debugging easier, the TURN server was written to also serve as
>>> a guide for others.
>>>
>>> ## Tested
>>> Every commit is tested via travis-ci Go provides fantastic facilities for
>>> testing, and more will be added as time goes on.
>>>
>>> ## Shared libraries
>>> Every pion product is built using shared libraries, allowing others to
>>> build things using existing tested STUN and TURN tools.
>>>
>>>
>>> If you are interested in using it, but it is missing a feature you need I
>>> would love to add it! The more users, the better the software gets.
>>>
>>>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
