Add headers for CreateSipParticipant #911
Conversation
🦋 Changeset detectedLatest commit: b68f3d4 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 |
| @@ -424,14 +424,22 @@ message CreateSIPParticipantRequest { | |||
| // If true, a random value for identity will be used and numbers will be omitted from attributes. | |||
| bool hide_phone_number = 10; | |||
|
|
|||
| // These headers are sent as-is and may help identify this call as coming from LiveKit for the other SIP endpoint. | |||
There was a problem hiding this comment.
I think we should just keep headers, and remove the mappings fields. the mappings are a level of indirection away and it's a bit difficult to understand.
There was a problem hiding this comment.
Makes sense! Made a new commit based on this comment
There was a problem hiding this comment.
lg, but will leave final approval to @biglittlebigben and @dennwc as we'll need to implement this in the SIP service
|
LGTM, but dennwc may have more opinions than me on this. |
There was a problem hiding this comment.
Look good, but you also need to update the NewCreateSIPParticipantRequest function. It should now merge headers from the trunk and the request.
rpc/sip.go
Outdated
| @@ -54,6 +54,11 @@ func NewCreateSIPParticipantRequest( | |||
| features = append(features, livekit.SIPFeature_KRISP_ENABLED) | |||
| } | |||
|
|
|||
| headers := req.Headers | |||
| if headers == nil { | |||
| headers = trunk.Headers | |||
There was a problem hiding this comment.
You'll have to iterate over key-values and add them to merge maps properly. Here you're discarding one of the maps.
There was a problem hiding this comment.
I see. My assumption was that user would be having headers in either CreateSIPParticipantRequest or SIPOutboundTrunkInfo, and if they had in both it would be the same- hence just overriding in case one is nil. Will make the requested change
|
@dennwc apologies as I completely missed this, thanks for fixing and getting this up! |
No description provided.