-
Notifications
You must be signed in to change notification settings - Fork 75
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
Implement TransferParticipant in SIP #425
Conversation
cmd/lk/sip.go
Outdated
Name: "transfer", | ||
Usage: "Transfer a SIP Participant", | ||
Action: transferSIPParticipant, | ||
ArgsUsage: RequestDesc[livekit.TransferSIPParticipantRequest](), |
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.
This API is pretty close to the ones we have on room participants in general, so maybe we should not require a JSON file here? Could be all 3 arguments instead: --room
, --id
(or how do we call participant identity in CLI?) and regular argument for the transfer URI.
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 using named args. We usually use --identity
for participants, and could map the rooms to --from
and --to
.
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.
To be consistent with other commands, should we use --room
, --identity
--to
? It may be confusing to have from
be a room, and to
a sip url
&cli.StringFlag{ | ||
Name: "to", | ||
Required: true, | ||
Usage: "`SIP URL` to transfer the call to. Use 'tel:<phone number>' to transfer to a phone", |
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.
Is the tel:
prefix a part of SIP spec? Did I miss the parsing part in the REFER
PR?
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.
This is supported and documented by Twilio. It's unclear if other providers support it as well. We can adjust the documentation as needed.
No description provided.