Add ManagedInstanceDTCs APIs #18770
Add ManagedInstanceDTCs APIs #18770
Conversation
|
Hi, @krivi37 Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vscswagger@microsoft.com |
Swagger Validation Report
|
| Rule | Message |
|---|---|
| The response of operation:'ManagedInstanceDtcs_Get' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDtcs.json#L62 |
|
| The response of operation:'ManagedInstanceDtcs_CreateOrUpdate' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDtcs.json#L113 |
️⚠️Avocado: 1 Warnings warning [Detail]
| Rule | Message |
|---|---|
| The default tag contains multiple API versions swaggers. readme: specification/sql/resource-manager/readme.md tag: specification/sql/resource-manager/readme.md#tag-package-composite-v5 |
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️Cross-Version Breaking Changes succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
️️✔️PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
ghost
added
the
Swagger Generation Artifacts
|
|
Hi @krivi37, Your PR has some issues. Please fix the CI sequentially by following the order of
|
|
systemData is not mandatory in the SQL spec |
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
Hi, @krivi37 your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board(armapireview@microsoft.com). |
...ation/sql/resource-manager/Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDTCs.json
Outdated
Show resolved
Hide resolved
| }, | ||
| "allowOutboundEnabled": { | ||
| "description": "Allow Outbound traffic of managed instance DTC.", | ||
| "type": "boolean" |
There was a problem hiding this comment.
consider updating these boolean values to enum as explained here :
https://armwiki.azurewebsites.net/rp_onboarding/process/api_review_best_practices.html
"Replace boolean/switch properties with better enum"
There was a problem hiding this comment.
Is this really needed? These values should only have True and False as values, and from what I see on the linked post:
"Even if you still believe [True, False] are the correct values for a property, you should use a string enum with values [True, False] instead of boolean. Enums are always a more flexible and future proof option because they allow additional values to be added in the future in a non-breaking way, e.g. [True, False, Unknown]."
These values are optional, so not setting them is not a problem (in other words, we don't need the "Unknown" option). Also, since they just determine whether some DTC options are enabled, they can only be "true" or "false". They also cannot be aggregated into a single property since they deal with logically different things.
There was a problem hiding this comment.
its not mandatory , but in general is a good rule of thumb. Often we see as a service grows , modelling things as boolean puts you in a corner when the scenario scope expands and you end up needing to have another bool to model something else. In your case it could be something like maybe 3 months down the line you need to have a new state like "allowInboundFromAzureStorage" (hypothetical , I do not know the specifics of your service) and then that ends up becoming another bool rather than an enum which is what it should be. I will not block sgn off on this comment , but would still would urge you to consider fixing this.
...ation/sql/resource-manager/Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDTCs.json
Outdated
Show resolved
Hide resolved
...ger/Microsoft.Sql/preview/2022-02-01-preview/examples/ManagedInstanceDtcUpdateEnableDtc.json
Show resolved
Hide resolved
raosuhas
added
the
ARMChangesRequested
| "description": "Managed instance DTC settings.", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/ManagedInstanceDtc" |
There was a problem hiding this comment.
The name cannot be changed, it is can only be "current" since this is a singleton object, and it is only used as a route in the request. The only properties that are actually getting updated are the ones in the body of the request. The dtcName is in the URL itself
There was a problem hiding this comment.
The ones in the body of the request, meaning the "ManagedInstanceDtcProperties" in this case? Think Suhas is suggesting to create a definition and include those that you'll update.
There was a problem hiding this comment.
Hmm, but everything which is listed under ManagedInstanceDtc -> ManagedInstanceDtcProperties can be patched. All the properties are mutable except for "dtcHostNameDnsSuffix" which is declared as read only.
There was a problem hiding this comment.
Overall it's just not a good pattern, as user can still pass them, but they are useless. It's a strong recommendation but will not block you on this.
There was a problem hiding this comment.
Took a second look on this, if ManagedInstanceDtcProperties is all you needed for patch, why not just pass them?
...ation/sql/resource-manager/Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDtcs.json
Outdated
Show resolved
Hide resolved
| }, | ||
| "202": { | ||
| "description": "Applying DTC settings is in progress" | ||
| } |
There was a problem hiding this comment.
Are you planning to support both sync and async for 200 and 202? For async patch, the recommend pattern is to use 202 and location header. See: https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/async-api-reference.md#updating-using-patch #Closed
There was a problem hiding this comment.
Shouldn't swagger document the final Location response which is 200 OK?
There was a problem hiding this comment.
We are following https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/async-api-reference.md#202-accepted-and-location-headers
Specifically:
5.If the operation is complete, return the exact same response that would have been returned had the operation been executed synchronously.
Swagger/AutoRest needs some way of determining the structure of the final response, and that is done by providing the sync response format here.
...source-manager/Microsoft.Sql/preview/2022-02-01-preview/examples/ManagedInstanceDtcList.json
Show resolved
Hide resolved
krivi37
force-pushed
the
dev/skrivokapic/addManagedInstanceDTCsAPIspecs
branch
from
8f00063 to
673a8ac
Compare
...ation/sql/resource-manager/Microsoft.Sql/preview/2022-02-01-preview/ManagedInstanceDtcs.json
Show resolved
Hide resolved
jorgecotillo
added
ARMSignedOff
openapi-workflow-bot
bot
added
ARMReview
and removed
WaitForARMFeedback
|
This API is special in the sense that the proxy resource is created by the RP, but the intention is to expose updatable properties via PUT, initially a PATCH API was exposed but didn't make much sense because both request bodies were identical, since the intention is to expose all properties as updatable properties and not only a small subset, PATCH is not required. |
|
@raych1 , can you please review and merge this PR? Thanks! |
* Added swagger, examples and changed the readme.md file * Fixed spelling errors and examples * Added a patch request option and renamed the file to lowercase * Update readme.md * Renamed examples for put and patch update * Extracted resource properties for patch request * Added new examples and changed existing ones * Added provisioning state to put request * Updated put examples * Changed provisioning status in the update examples * Updated provisioning state enum * Removed 202 code from PUT provisioning state response and updated examples accordingly * Removed patch operation and associated examples, also renamed put examples * Changed descriptions of some models * Renamed an example description Co-authored-by: Stefan Krivokapic <skrivokapic@microsoft.com>
* Added swagger, examples and changed the readme.md file * Fixed spelling errors and examples * Added a patch request option and renamed the file to lowercase * Update readme.md * Renamed examples for put and patch update * Extracted resource properties for patch request * Added new examples and changed existing ones * Added provisioning state to put request * Updated put examples * Changed provisioning status in the update examples * Updated provisioning state enum * Removed 202 code from PUT provisioning state response and updated examples accordingly * Removed patch operation and associated examples, also renamed put examples * Changed descriptions of some models * Renamed an example description Co-authored-by: Stefan Krivokapic <skrivokapic@microsoft.com>
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Add a changelog entry for this PR by answering the following questions:
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label "WaitForARMFeedback" will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you are using OpenAPIHub to initialize the PR for adding a new version. More details, refer to the wiki.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.