Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.69k stars 5.11k forks source link

Add api version 2024-10-01-preview for Azure SignalR and Azure Web PubSub #31544

Open bjqian opened 1 week ago

bjqian commented 1 week ago

ARM (Control Plane) API Specification Update Pull Request

[!TIP] Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

spec_pr_review_workflow_diagram

Purpose of this PR

What's the purpose of this PR? Check the specific option that applies. This is mandatory!

Due diligence checklist

To merge this PR, you must go through the following checklist and confirm you understood and followed the instructions by checking all the boxes:

Additional information

Viewing API changes For convenient view of the API changes made by this PR, refer to the URLs provided in the table in the `Generated ApiView` comment added to this PR. You can use ApiView to show API versions diff.
Suppressing failures If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the [suppressions guide](https://aka.ms/azsdk/pr-suppressions) to get approval.

Getting help

openapi-pipeline-app[bot] commented 1 week ago

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.
openapi-pipeline-app[bot] commented 1 week ago

Generated ApiView

Language Package Name ApiView Link
Go sdk/resourcemanager/signalr/armsignalr https://apiview.dev/Assemblies/Review/454fa36be1744f6282a36247b001738e?revisionId=e29b3ea92c0a4b1eb09c9c35c656866b
Go sdk/resourcemanager/webpubsub/armwebpubsub https://apiview.dev/Assemblies/Review/063845482f6d4a84b8cd66b982770577?revisionId=9186879460d4412fb1da9441fce3c0a1
.Net Azure.ResourceManager.WebPubSub There is no API change compared with the previous version
JavaScript @azure/arm-signalr https://apiview.dev/Assemblies/Review/19dcf3a0cb214c9eb26656d78f00c556?revisionId=30dfc09302ef4d5491adc36dff996910
JavaScript @azure/arm-webpubsub https://apiview.dev/Assemblies/Review/a8172261dd334c3c939f4c623fd83907?revisionId=ceec4f73791643019042f61df4a3eb0a
Python azure-mgmt-signalr https://apiview.dev/Assemblies/Review/b760607d857b4332aa3d2893df068ed5?revisionId=ab5074bac23c49edafc5ee14dfc61f62
Python azure-mgmt-webpubsub https://apiview.dev/Assemblies/Review/f8c77e10962543998f8e9d2357fd7a1e?revisionId=00a9af9da9f44de484550697fea31d0e
Java azure-resourcemanager-signalr https://apiview.dev/Assemblies/Review/aa1e87da06d14e96ade025aacaab4de3?revisionId=94c7924aa34a4d44bce03a21803c72e6
Java azure-resourcemanager-webpubsub There is no API change compared with the previous version
Swagger Microsoft.SignalRService https://apiview.dev/Assemblies/Review/c19018e1a4434887ae6482edbd28f151?revisionId=9905a15f99e54594bd5a864984d1f0c0
razvanbadea-msft commented 1 week ago
    "maxInboundMessageBytes": {

if there is a 1GB as default why did you not set it? like you added for aggregationWindowInSeconds


Refers to: specification/signalr/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/signalr.json:3743 in beb58ba. [](commit_id = beb58bae6ea20745963ae7853c3f034e46469f15, deletion_comment = False)

razvanbadea-msft commented 1 week ago
      "description": "Gets or sets the Keep-Alive Interval. Optional to set.\r\nValue is in seconds.\r\nThe default value is 15 seconds.\r\nCustomers should set this value to a shorter period if they want the service to send keep-alive messages more frequently, \r\nensuring timely checks of the connection status.\r\nConversely, customers can set this value to a longer period if they want the service to send keep-alive messages less frequently, \r\nreducing network traffic, but note that it may take longer to detect a disconnection.\r\nThis interval ensures that the connection is maintained by sending periodic keep-alive messages to the client.",

same thing about adding also a default


Refers to: specification/signalr/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/signalr.json:3044 in beb58ba. [](commit_id = beb58bae6ea20745963ae7853c3f034e46469f15, deletion_comment = False)

razvanbadea-msft commented 1 week ago
      "description": "Rules to control the client traffic",

you can specify in the description that you cant add more than 10


Refers to: specification/signalr/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/signalr.json:2157 in beb58ba. [](commit_id = beb58bae6ea20745963ae7853c3f034e46469f15, deletion_comment = False)

razvanbadea-msft commented 1 week ago
    "maxInboundMessageBytes": {

same comment in this file about default attribute


Refers to: specification/webpubsub/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/webpubsub.json:3776 in beb58ba. [](commit_id = beb58bae6ea20745963ae7853c3f034e46469f15, deletion_comment = False)

bjqian commented 1 week ago
      "description": "Gets or sets the Keep-Alive Interval. Optional to set.\r\nValue is in seconds.\r\nThe default value is 15 seconds.\r\nCustomers should set this value to a shorter period if they want the service to send keep-alive messages more frequently, \r\nensuring timely checks of the connection status.\r\nConversely, customers can set this value to a longer period if they want the service to send keep-alive messages less frequently, \r\nreducing network traffic, but note that it may take longer to detect a disconnection.\r\nThis interval ensures that the connection is maintained by sending periodic keep-alive messages to the client.",

same thing about adding also a default

Refers to: specification/signalr/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/signalr.json:3044 in beb58ba. [](commit_id = beb58ba, deletion_comment = False)

Hi @razvanbadea-msft . In this PR review , I found a new rule: image It's telling us to remove the swagger default value in the patch body. I'm confused whether or not we should mark the default value in the swagger. 😂

razvanbadea-msft commented 1 week ago
      "description": "Gets or sets the Keep-Alive Interval. Optional to set.\r\nValue is in seconds.\r\nThe default value is 15 seconds.\r\nCustomers should set this value to a shorter period if they want the service to send keep-alive messages more frequently, \r\nensuring timely checks of the connection status.\r\nConversely, customers can set this value to a longer period if they want the service to send keep-alive messages less frequently, \r\nreducing network traffic, but note that it may take longer to detect a disconnection.\r\nThis interval ensures that the connection is maintained by sending periodic keep-alive messages to the client.",

same thing about adding also a default Refers to: specification/signalr/resource-manager/Microsoft.SignalRService/preview/2024-10-01-preview/signalr.json:3044 in beb58ba. [](commit_id = beb58ba, deletion_comment = False)

Hi @razvanbadea-msft . In this PR review , I found a new rule: image It's telling us to remove the swagger default value in the patch body. I'm confused whether or not we should mark the default value in the swagger. 😂

for patch operations: A recommended way is to define a new model that only contains the patchable properties to replace the original parameter in request body. - https://github.com/Azure/azure-openapi-validator/blob/main/docs/patch-body-parameters-schema.md#how-to-fix

are those properties patchable or can be excluded from it?

bjqian commented 3 days ago

Hi @razvanbadea-msft .

Thanks for the clarification! I understand the recommended approach is to use a separate model for patch if a model contains "default/required" properties.

Let me elaborate a bit on the issue:

Yes, those properties are pachable. However, the two properties in question were introduced in a model from a previous API version. Switching to a new model could potentially break existing client code.

In our Resource Provider, the actual behavior for default value in the comment is as follows:

In this way, we could avoid introducing "swagger default" in the client side.

Thank you again for highlighting this issue! Please let us know if you'd like more details or further clarification.