Skip to content

Conversation

@nishadmusthafa
Copy link
Contributor

No description provided.

Copy link
Member

@rektdeckard rektdeckard left a comment

Choose a reason for hiding this comment

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

LGTM.

I really want us to rethink if using the same PhoneNumber model for inventory and owned numbers is a good idea. This info should be able to be preloaded on numbers response, not stitched together in each client of the data. We already do something similar in dashboard and it is painful and error prone.

sipClient := lksdk.NewSIPClient(project.URL, project.APIKey, project.APISecret, withDefaultClientOpts(project)...)

// List all dispatch rules
resp, err := sipClient.ListSIPDispatchRule(ctx, &livekit.ListSIPDispatchRuleRequest{})
Copy link
Member

Choose a reason for hiding this comment

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

Just noting, we still haven't resolved the issue with max twirp message size, either here or in SDK. Probably very few customers hitting that but we need to come back to the idea of auto-paginating results when we do things like this.

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 similar to what we've done here -> https://github.com/livekit/livekit-cli/pull/694/files ? I can take that up after landing this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that was the idea for a short-term fix. What we probably want to for real tho is something like this if the results are requested in JSON format (complete results are expected) but have real pagination in the CLI for when you expect a table format (just call with nextPageToken on arrow key or something).

@nishadmusthafa nishadmusthafa merged commit 1454db9 into main Nov 19, 2025
9 checks passed
@nishadmusthafa nishadmusthafa deleted the phone-number-assigment-change branch November 19, 2025 20:39
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.

3 participants