-
Notifications
You must be signed in to change notification settings - Fork 14
WIP - Convert Fabric APIs to TypeSpec #373
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for signalwire-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
yarn run v1.22.22 OpenAPI Specification AnalysisChanges are compared to the
Done in 152.35s. |
Should fix some of the errors in the OpenAPI report while im already here and working on this |
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'm very sorry for all the comments, please feel free to ask for clarification on anything! I know I made a few as notes and I will come back to those shortly.
I think this is looking great overall! I think there are some copy paste issues, and a few endpoints with the wrong bodies.
These also don't feel clearly represented to me, but I might have missed it:
@summary("Application Address") | ||
model FabricAddressApp extends FabricAddress { | ||
@doc("Type of the Fabric Address which is an application.") | ||
@example(DisplayTypes.App) | ||
type: DisplayTypes.App; | ||
} | ||
|
||
@summary("Room Address") | ||
model FabricAddressRoom extends FabricAddress { | ||
@doc("Type of the Fabric Address which is a conference room.") | ||
@example(DisplayTypes.Room) | ||
type: DisplayTypes.Room; | ||
} | ||
|
||
@summary("Call Address") | ||
model FabricAddressCall extends FabricAddress { | ||
@doc("Type of the Fabric Address which is a call.") | ||
@example(DisplayTypes.Call) | ||
type: DisplayTypes.Call; | ||
|
||
} | ||
|
||
@summary("Subscriber Address") | ||
model FabricAddressSubscriber extends FabricAddress { | ||
@doc("Type of the Fabric Address which is a subscriber.") | ||
@example(DisplayTypes.Subscriber) | ||
type: DisplayTypes.Subscriber; | ||
} |
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.
thought: The phrasing of these is a little confusing to me, but I'm not coming up with any alternatives off the bat. Leaving a note here so I come back to it
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.
After further evaluation, I think the confusing part to me is "which is a(n) X". The display types are only for presenting details in the UI and filtering results.
I'm not sure what we could say about these two. It might be best to talk to product or Bryan to understand the best phrasing for these.
@summary("Application Address")
@doc("Type of the Fabric Address which is an application.")
@summary("Call Address")
@doc("Type of the Fabric Address which is a call.")
I think we could rephrase this as "the display type of a fabric address pointing to a conference room".
@summary("Room Address")
@doc("Type of the Fabric Address which is a conference room.")
I think we could rephrase this as "the display type of a fabric address pointing to a subscriber".
@summary("Subscriber Address")
@doc("Type of the Fabric Address which is a subscriber.")
What are your thoughts?
subscriber_id: uuid; | ||
|
||
@doc("The token that is associated with the subscriber.") | ||
@format("uuid") |
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.
suggestion (non-blocking): I think "jwt" for the format
@example("eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIiwidHlwIjoiU0FUIn0..HahMYxqt4uI14qSH.daMTBR53lfEfEFiVAhr0pPSRqZhEod_YzavoG9-4ieiRQvl8GtP3FFNx0VLfkJqNcjUNbAaiKrEMnfOtCnQjiq1Kn0Iq90MYdM00QJ7cTaQ88vfbqdE92p-d4oDeg6z_vAsgrFgEobmrlDQndKxCWOD921iYxyLP0vqNaokN3kIM06iAWu_UpnTYEeR1l068xhK2xb6P9wbI2FDKFQoMgCdbjvABF7RRyaEzUoaQ5_Wj53YO6PFYuYcPbqMhdtvSSQiK3Nw6bFer2OfFs6s2RTukRGsocgC5Q7pwQwzYky-YgrPCb-pVAJajVSXUJrayvOi8-TeyCpICW4zTeJa5icZ380cWtafUH4rEB_FOJciJf0BCy48ajbz0NE121uBl2mqA1HE0_mQA53UqVjbrbE9hVOfnN4KpwOfULhIjx54tIekJQgG-aK2AYsLPCDNhuSpHvdwJcTM0Gzy3mS2veyaDV8q2qN5F_F9OThTQzcfy.AXzVNrJc_pGVPsticsVM0w") | ||
token: uuid; | ||
|
||
@doc("Refresh token.") | ||
@format("uuid") |
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.
suggestion (non-blocking): I think "jwt" for the format
@tag("SWML Scripts") | ||
interface SWMLScriptAddresses { | ||
@summary("List SWML Script Addresses") | ||
@doc("A list of SWML Script Addresses. This endpoint uses the bearer token authentication method with the SAT (Subscriber Access Token) which can be generated using the [Create Subscriber Token endpoint](/rest/signalwire-rest/endpoints/fabric/subscriber-tokens-create).") |
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.
issue: This endpoint uses basic auth
|
||
@doc("Optional description of the SWML script") | ||
@example("Welcome message script") | ||
description?: string; |
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.
question: I don't think this is a field we have, did you see this somewhere?
@@ -13,7 +13,7 @@ namespace FabricAPI.SWMLWebhookAddresses { | |||
@friendlyName("SWML Webhooks") | |||
interface SWMLWebhookAddresses { | |||
@summary("List SWML Webhook Addresses") | |||
@doc("A list of SWML Webhook Addresses") | |||
@doc("A list of SWML Webhook Addresses. This endpoint uses the bearer token authentication method with the SAT (Subscriber Access Token) which can be generated using the [Create Subscriber Token endpoint](/rest/signalwire-rest/endpoints/fabric/subscriber-tokens-create).") |
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.
issue: This endpoint uses basic auth
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.
Looking really great!
Few concerns:
@tag("FreeSWITCH Connectors") | ||
interface FreeswitchConnectorAddresses { | ||
@summary("List FreeSWITCH Connector Addresses") | ||
@doc("A list of FreeSWITCH Connector Addresses. This endpoint uses the bearer token authentication method with the SAT (Subscriber Access Token) which can be generated using the [Create Subscriber Token endpoint](/rest/signalwire-rest/endpoints/fabric/subscriber-tokens-create).") |
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 think this endpoint does use basic auth
@@ -1,5 +1,6 @@ | |||
model SubscriberRefreshTokenRequest { | |||
@doc("The refresh token previously issued alongside a subscriber access token. This token is used to request a new access token.") | |||
@example("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...") | |||
refresh_token: string; | |||
@format("uuid") | |||
refresh_token: uuid; |
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.
Same concern as Cassie regarding the format
@@ -1,9 +1,11 @@ | |||
model SubscriberRefreshTokenResponse { | |||
@doc("A newly generated subscriber access token, valid for 2 hours.") | |||
@example("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...") | |||
token: string; | |||
@format("uuid") | |||
token: uuid; |
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.
Same concern as Cassie regarding the format
|
||
@doc("A new refresh token, valid for 2 hours and 5 minutes.") | ||
@example("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...") | ||
refresh_token: string; | ||
@format("uuid") | ||
refresh_token: uuid; |
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.
Same concern as Cassie regarding the format
REST API Update Pull Request
Description
Convert all Fabric APIs to TypeSpec
Type of Change
Motivation and Context
We want to live in a world of only TypeSpec
Checklist:
team/developer-experience
label in the PR.