Codecov Report

❌ Patch coverage is 85.71429% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.17%. Comparing base (e3a1986) to head (61380d5).
⚠️ Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
+ Coverage   84.13%   84.17%   +0.03%     
==========================================
  Files          54       54              
  Lines        4287     4366      +79     
==========================================
+ Hits         3607     3675      +68     
- Misses        494      500       +6     
- Partials      186      191       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I think this is now independent of options/config but still a breaking change 😨

@fippo this should be merged after or base on #450 as an option With[name] we're going to deprecate this config struct. We shouldn't break the API. I think @philipch07 is going to land this in few days.

@JoTurk problem is that sctp-init (and maxMessageSize + sctp port) should not be a config option, these are negotiated parameters. https://draft.ortc.org/#sctp-transport* had them as "capabilities" but that distinction is too fine-grained (as shown by the non-object remotePort argument)

@fippo I get the point in the WebRTC context that these inputs are negotiated/signaled values.

But pion/sctp as well as most of our libraries are intentionally standalone. it doesn't own SDP/signaling, We try to make our libraries less aware of the webrtc semantics as much as possible, so the only thing it should do is accept those negotiated values as inputs from the caller. In some applications these valuse can be provided by the application for example application with static maxMessageSize.

Exposing them via options patterns doesn't mean "the app must pick them" and shouldn't negotiate them, it's just the mechanism for providing externally parameters to library, config or negotiated parameters, or anything else, that's all. we are adopting this pattern across all of pion libraries, and we have many users that aren't webrtc users.

Noting here so I don't forget. I have a local rebase but will wait with pushing.

func WithSNAP(localSctpInit []byte, remoteSctpInit []byte) AssociationOption { ... }

options got merged, sorry for the conflicts.

not sure if comparing config's (which will be internal at some point anyway?) is needed

(casting to strings was 😭 but...)

@fippo I try not to be rushed with the reviews if it's still pending / experimental work, but you can ping me if you want quick or actual review, just making sure you're not being ghosted :)

A review would be great but it is probably best to not to merge until some IETF folks have read the draft ;-)

is this patch ready to go?

heh, the timing of the options migration got indeed in the way but it is what it is. Thank you for the review(s) and the patience!

(the snap branch in pion/webrtc should have most of what the migration needs, feel free to borrow)