-
Notifications
You must be signed in to change notification settings - Fork 113
Lk phone numbers #1146
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
Lk phone numbers #1146
Conversation
🦋 Changeset detectedLatest commit: 89c753b 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 |
7b9b4ca to
dc846fd
Compare
| import "google/protobuf/empty.proto"; | ||
|
|
||
| // Public Phone Number Service - External API for phone number management | ||
| service PhoneNumberService { |
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.
If this makes it to OSS, do you think we could adapt it to Twilio API for example?
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.
yeah, I think that's a good idea. Would take a stab at it in a later iteration. Also as I mentioned in the comment below, we might have variations in how that is implemented in cloud/oss for things like what is supported. Billing probably is not something we might support in OSS as it would be something very custom to implementation.
dennwc
left a comment
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.
Looks good now, thanks a lot! One small item remains only.
…nd pricing for the same
279c05d to
27bd4b1
Compare
davidzhao
left a comment
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.
it'd be great if we can simplify the PurchasedPhoneNumber, GlobalPhoneNumber, and PhoneNumberInventoryItem.
can they be the same object?
| } | ||
|
|
||
| // TelephonyCost represents the pricing structure for a specific telephony service | ||
| message TelephonyCost { |
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.
I don't think this should be exposed. our other APIs don't expose cost/pricing information currently.
when we do, it should not be dependent on a particular resource. but rather types of resources.
| returns (PurchasePhoneNumberResponse) {} | ||
|
|
||
| // List purchased phone numbers for a project | ||
| rpc ListPurchasedPhoneNumbers(ListPurchasedPhoneNumbersRequest) |
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.
| rpc ListPurchasedPhoneNumbers(ListPurchasedPhoneNumbersRequest) | |
| rpc ListPhoneNumbers(ListPhoneNumbersRequest) |
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.
IMO by default list is listing resources that you own
| PhoneNumberStatus status = 2; // Current status of the phone number | ||
| google.protobuf.Timestamp assigned_at = 3; // Timestamp when the number was assigned | ||
| google.protobuf.Timestamp released_at = 4; // Timestamp when the number was released (if applicable) | ||
| SIPDispatchRuleInfo sip_dispatch_rule = 5; // Optional: Associated SIP dispatch rule |
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.
should this be set on the dispatch rule itself? we already have the "inbound numbers" field in there
No description provided.