Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.

MIT License

2.68k stars 5.11k forks source link

[Communication Services - NetworkTraversal] API Stewardship Review #16755

Closed garchiro7 closed 2 years ago

garchiro7 commented 3 years ago

Review with the Azure Board.

Key contact: anpea@microsoft.com

Swagger definition

Scenarios:

Several object renames (e.g. "Turn" to "Relay" and "TurnServer" to "IceServer")
Path replacement from "/turn/{id}/:issueCredentials" to "/networktraversal/:issueRelayConfiguration"
Adds a CommunicationRelayConfigurationRequest, making the identity optional - Adds a RouteType to IceServers to be requested

markweitzel commented 2 years ago

API Stewardship Rewview 18-Oct-21

@ajpeacock0 & @jorgegarchirota all looks good. Minor stuff to fix...

Parameter should have a description. (line 26)
The body parameter is not marked as required. (line 26)
Error response should contain a x-ms-error-code header. (line 41)

ajpeacock0 commented 2 years ago

API Stewardship Rewview 18-Oct-21

@ajpeacock0 & @jorgegarchirota all looks good. Minor stuff to fix...

Parameter should have a description. (line 26)

The body parameter is not marked as required. (line 26)

Error response should contain a x-ms-error-code header. (line 41)

@markweitzel For "The body parameter is not marked as required. (line 26)" this was done since the two properties in CommunicationRelayConfigurationRequest are optional, so in the case the caller does not want to provide on, they wouldn't be forced to give an empty json object.

Are you saying this optional request must be required, thus making the caller inclide a {} body?

mikekistler commented 2 years ago

Regarding the optional request body, if it is truly optional then this is fine. This is often an oversight and that's why we flag it. But I wonder -- in the even that the body properties are not provided, what is the behavior of the API? There is no "default" value for these properties, so how will users know what to expect if they don't specify them?

ajpeacock0 commented 2 years ago

There is no "default" value for these properties, so how will users know what to expect if they don't specify them?

That's a good point. Having no default was also intentional, since for id that doesn't make sense and for routeType we have the behavior of returning all values if nothing is specified, but we don't indicate this. I'll add to the description for both to explain behavior