Skip to content

Conversation

@dennwc
Copy link
Contributor

@dennwc dennwc commented Jun 17, 2024

Support new SIP Inbound/Outbound Trunk API and move all SIP commands into a separate cli tree:

  • list-sip-trunk -> sip trunk list
  • list-sip-dispatch-rule -> sip dispatch list
  • create-sip-participant -> sip participant create

Old commands continue to work, they are only hidden from the CLI help.

I also renamed the columns for legacy trunks list command. Now they match names in new API (OutboundNumber -> Number, InboundNumbers -> AllowedNumbers).

Requires livekit/protocol#738

@dennwc
Copy link
Contributor Author

dennwc commented Jun 17, 2024

This should wait for prod deployment. It hides old CLI commands, while new ones won't work without updated server.

@davidzhao
Copy link
Member

@rektdeckard what do you think about the subcommand organization? this is something we'd also want to make consistent across the board.

@dennwc
Copy link
Contributor Author

dennwc commented Jun 21, 2024

Regarding sub-commands, it would be also nice to move shared options like URL/key/secret to the root command:

livekit-cli --api-key <key> --api-secret <secret> some sub command

instead of

livekit-cli some sub command  --api-key <key> --api-secret <secret>

@rektdeckard
Copy link
Member

rektdeckard commented Jun 21, 2024

@davidzhao @dennwc looks like what what we had in mind, and consistent with prior art in the projects subcommand. Great shout on moving the global parameters too! Other main thing to think about IMO is making the key argument a positional argument, for example:

livekit-cli room join "my-first-room"

instead of

livekit-cli join-room --room "my-first-room"

Since the room is the main and only logical receiver of the command. I don't know if the --request flag qualifies here or not. WDYT?

@dennwc
Copy link
Contributor Author

dennwc commented Jun 21, 2024

Good idea! Will make main arg positional then for new SIP commands 👍

Regarding flags, that should probably be in a separate PR, because existing commands will need to be updated too.

@dennwc
Copy link
Contributor Author

dennwc commented Jun 25, 2024

@rektdeckard Updated new SIP commands to accept main args (id or request).

Moving flags to the top will require an update to github.com/urfave/cli/v3, which added Persistent to the flag definitions. Could be a separate PR.

@rektdeckard
Copy link
Member

Nice! Agree, separate PR for that. v2 seems to be in maintenance mode, so probably a good thing regardless?

return createAndPrintLegacy(c, func(ctx context.Context, req *livekit.CreateSIPParticipantRequest) (*livekit.SIPParticipantInfo, error) {
// CreateSIPParticipant will wait for LiveKit Participant to be created and that can take some time.
// Default deadline is too short, thus, we must set a higher deadline for it.
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably outside my wheelhouse, but when docs are updated it may be good to include constants like this if it is invariant, or expose it as a parameter if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I can actually bake it into the SDK, so not configuration will be needed.

@dennwc dennwc merged commit 8f795dd into main Jun 25, 2024
@dennwc dennwc deleted the sip-trunks-v2 branch June 25, 2024 20:55
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.

5 participants