Skip to content

Conversation

@biglittlebigben
Copy link
Contributor

No description provided.

cmd/lk/sip.go Outdated
Name: "transfer",
Usage: "Transfer a SIP Participant",
Action: transferSIPParticipant,
ArgsUsage: RequestDesc[livekit.TransferSIPParticipantRequest](),
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor Author

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",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@biglittlebigben biglittlebigben merged commit 93657b7 into main Sep 25, 2024
@biglittlebigben biglittlebigben deleted the benjamin/sip_transfer branch September 25, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants