-
Notifications
You must be signed in to change notification settings - Fork 123
Split SIP Trunk configuration to inbound and outbound parts #738
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
Conversation
🦋 Changeset detectedLatest commit: 44fd60d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
davidzhao
left a comment
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.
lgtm! this seems cleaner and easier to understand.
protobufs/livekit_sip.proto
Outdated
| SIPTransport transport = 5; | ||
|
|
||
| // A number used to make the calls. | ||
| string number = 6; |
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.
On the trunking provider side, it's still possible to associate multiple numbers to the SIP trunk. but I guess the point here is when making a call, a particular number will always be used?
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.
Yes, it's possible to use multiple numbers for outbound on the provider's side.
Exactly, this is to specify which specific number to use. Having said that, I was also considering an array here. The implementation could pick a random number from an array before making a call.
Lets allow multiple outbound numbers then.
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.
so maybe for the protocol, it'd be better to leave it as a repeated field? but in usage, we would let people know that only the first is used?
in the future, we can then add ways of choosing a trunk at run-time
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.
Already updated to repeated field 👍 It will actually use the random one from the list if the Trunk is created with new API. And if not - it will be the same as before (one number only).
|
I'll keep it open for a bit longer - preparing PRs to other repos. |
|
Works well, merging |
Initial decisions made on the naming of the fields in
SIPTrunkInfoleads to a lot of confusion for our users.Most importantly, the
outbound_numberfield is actually used for both inbound and outbound as the number associated with this LK SIP Trunk. Ideally it should be namednumber.Unfortunately, we cannot rename it without breaking compatibility. This is mainly because our CLI uses
protojsonfor decoding these fields. Changing the name will break old JSON request files.Some users also asked if it's possible to map multiple numbers to one (inbound) Trunk. Again, we cannot change the type of the field, but we could introduce a new one. At the same time, multiple numbers may be unnecessary for outbound Trunks, but they will still appear there because the structure is shared for inbound/outbound.
At the end, it's clear that there's no reason to share inbound and outbound configuration in one entity. Thus, this change splits Trunk into inbound and outbound configs.
Specifically:
SIPInboundTrunkInfoandSIPOutboundTrunkInfo, add deprecation note forSIPTrunkInfo.We will keep DB structure mostly the same and convert objects in flight. One column will be added:
kind. New Trunks will have it set to either inbound or outbound, so that new objects appear in one list only, while old ones appear in both. Eventually we could migrate to separate DB tables.