search

pion / stun

A Go implementation of STUN
https://pion.ly/
MIT License
625 stars 94 forks source link

Update cmd/stun-nat-behaviour strictly following RFC5780 & RFC4787 #36

Closed bixycler closed 4 years ago

bixycler commented 4 years ago

Description

Reference issue

Previous pull request: #33

Sean-Der commented 4 years ago

Amazing work @bixycler, will review now. Sorry for the delay :(

Sean-Der commented 4 years ago

@bixycler What do you think of it always being verbose (and removing options)? Since this is a debugging tool we can split out the info into separate sections.

But less arguments if possible is better IMO

Would you also mind reviewing the golangci-lint (version 1.19.1) errors and running go test ./... we seem to have test failures. thanks!

bixycler commented 4 years ago

As @Sean-Der requested, I've removed the point "Make the output clearer with 3 levels of verbosity."

I'm sorry for many forced pushes, but they are just to fix many checking failures due to the strict rules.

Finally, the remaining failure is "File is not goimports-ed" which is very confusing. Maybe it's complaining about the many spaces inserted to align the code for good looking.

bixycler commented 4 years ago

Finally, the remaining failure is "File is not goimports-ed" which is very confusing.

With the newer version of golangci-lint (1.28.2), I see a clearer message about that error: "File is not gofmt-ed with -s".

cmd/stun-nat-behaviour/main.go:29: File is not `gofmt`-ed with `-s` (gofmt)
    addrStrPtr  = flag.String("server", "stun.voip.blackberry.com:3478", "STUN server address")
    timeoutPtr  = flag.Int("timeout", 3, "the number of seconds to wait for STUN server's response")
    ErrTimedOut = errors.New("timed out waiting for response")

Even though -s is about simplifying code but it does align all var lists following to its own rule! So, I just re-align them following gofmt -s anyway.

codecov[bot] commented 4 years ago

Codecov Report

Merging #36 into master will decrease coverage by 0.65%. The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   99.50%   98.84%   -0.66%     
==========================================
  Files          19       19              
  Lines        1204     1212       +8     
==========================================
  Hits         1198     1198              
- Misses          4       12       +8     
  Partials        2        2
Flag Coverage Δ
#go 98.84% <44.44%> (-0.66%) :arrow_down:
#wasm 55.85% <44.44%> (-0.38%) :arrow_down:
Impacted Files Coverage Δ
attributes.go 95.65% <0.00%> (ø)
addr.go 85.91% <50.00%> (-10.91%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8c0b364...6387385. Read the comment docs.

Sean-Der commented 4 years ago

This is quite an amazing PR, @bixycler easily one of the best 'First PRs' Pion has ever seen!

Would you be interested in joining the pion org? I would love to have your continued involvement in the project :) No real responsibility, but it would be really great to have who knows this stuff much deeper then me!

Sean-Der commented 4 years ago

I think I was wrong about removing the verbosity levels. I love all the extra info, but beginners will find it overwhelming.

I can take this PR, but if you are interested it would be great to bring back the levels. I just got caught up on all the verbose level checking. Maybe if we used the log package, and used log levels instead the code would be easier to follow?

bixycler commented 4 years ago

Maybe if we used the log package, and used log levels instead the code would be easier to follow?

I don't see the "levels" in Go's standard log package. But I'm trying logrus to implements the 3 levels of logging:

  1. Only results (mapping & filtering)
  2. [Default] The tests being carried out and then results
  3. (1) + details in STUN messages (both requests & responses)

Would you be interested in joining the pion org?

I've joined, haven't I? :open_mouth: ?! I've just changed my Pion's membership from "private" -> "public" so that everyone can see it.