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.
