Added new version in Microsoft.RecoveryServices and s360 fixups #14752
Conversation
|
Hi, @asmaskar 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. [email protected] |
|
[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. |
Swagger Validation Report
|
| Rule | Message |
|---|---|
|
Resource type naming must follow camel case. Path: '/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/certificates/{certificateName}' Location: Microsoft.RecoveryServices/stable/2021-04-01/registeredidentities.json#L21 |
|
Resource type naming must follow camel case. Path: '/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/registeredIdentities/{identityName}' Location: Microsoft.RecoveryServices/stable/2021-04-01/registeredidentities.json#L80 |
|
Resource type naming must follow camel case. Path: '/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/replicationUsages' Location: Microsoft.RecoveryServices/stable/2021-04-01/replicationusages.json#L21 |
|
Resource type naming must follow camel case. Path: '/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/usages' Location: Microsoft.RecoveryServices/stable/2021-04-01/vaultusages.json#L21 |
|
The response of operation:'VaultExtendedInfo_Get' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.RecoveryServices/stable/2021-04-01/vaults.json#L548 |
|
The response of operation:'VaultCertificates_Create' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.RecoveryServices/stable/2021-04-01/registeredidentities.json#L22 |
|
The response of operation:'VaultExtendedInfo_CreateOrUpdate' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.RecoveryServices/stable/2021-04-01/vaults.json#L592 |
|
The response of operation:'VaultExtendedInfo_Update' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.RecoveryServices/stable/2021-04-01/vaults.json#L648 |
|
The response is defined but without a default error response implementation.Consider adding it.' Location: Microsoft.RecoveryServices/stable/2021-04-01/registeredidentities.json#L64 |
|
The response is defined but without a default error response implementation.Consider adding it.' Location: Microsoft.RecoveryServices/stable/2021-04-01/registeredidentities.json#L108 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️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.
- Compared Swaggers (Based on Oad v0.8.11)
- current:stable/2021-04-01/registeredidentities.json compared with base:stable/2021-03-01/registeredidentities.json
- current:stable/2021-04-01/replicationusages.json compared with base:stable/2021-03-01/replicationusages.json
- current:stable/2021-04-01/vaults.json compared with base:stable/2021-03-01/vaults.json
- current:stable/2021-04-01/vaultusages.json compared with base:stable/2021-03-01/vaultusages.json
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️[Staging] SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
|:speech_balloon: AutorestCore/Exception|"readme":"recoveryservices/resource-manager/readme.md",
"tag":"package-2021-04",
"details":"> Installing AutoRest extension '@microsoft.azure/openapi-validator' (1.8.0)"|
|:speech_balloon: AutorestCore/Exception|"readme":"recoveryservices/resource-manager/readme.md",
"tag":"package-2021-04",
"details":"> Installed AutoRest extension '@microsoft.azure/openapi-validator' (1.8.0->1.8.0)"|
|:speech_balloon: AutorestCore/Exception|"readme":"recoveryservices/resource-manager/readme.md",
"tag":"package-2021-04",
"details":"> Loading AutoRest extension '@autorest/modelerfour' (4.15.456->4.15.456)"|
️️✔️[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️[Staging] Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
Swagger Generation Artifacts
|
|
@raych1 Can you please let me know if things are fine with this PR. I have just copied and added a new version. |
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
Hi, @asmaskar your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board([email protected]). |
|
I don't see any change in the versions! |
chiragg4u
added
the
ARMChangesRequested
|
Hi @chiragg4u a new api version which is 2021-04-01 is added, the files are just copied because we had to add a new api version. |
|
These changes are because we have 3 swagger documents and there was an addition of the api version in one of the swagger json and i was told that as per the guidelines if there is an addition in any one of them, we need to add the new version in the other 2 swagger. Hence these changes |
asmaskar
changed the title
|
Hi @chiragg4u I have added a few changes in this PR itself (which i was planning in a different PR), due to time constraints. The name was wrong in our swagger doc and i have rectified in this PR. |
| "application/json" | ||
| ], | ||
| "paths": { | ||
| "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/certificates/{certificateName}": { |
| ], | ||
| "paths": { | ||
| "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/certificates/{certificateName}": { | ||
| "put": { |
There was a problem hiding this comment.
Any reason for not including get, delete calls?, see: Inconsistent or incomplete CRUD from our documentation for more information, seems like you are missing those.
There was a problem hiding this comment.
Sorry as I was just adding the new api version, I just copied the existing json files without any changes. My changes are mainly in the vaults.json and not in the other 3 json swaggers
There was a problem hiding this comment.
I will still discuss this with the team but can this be taken later as this is the existing swagger and this change is outside the scope of the current changes?
There was a problem hiding this comment.
Got it, thanks for the clarification, will focus on the updates only.
| } | ||
| }, | ||
| "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/registeredIdentities/{identityName}": { | ||
| "delete": { |
There was a problem hiding this comment.
This is an existing swagger, as i have no change in this related to this PR, I have just copy pasted the existing one to the new api version
| "application/json" | ||
| ], | ||
| "paths": { | ||
| "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/replicationUsages": { |
There was a problem hiding this comment.
I have just copied the existing swagger to the new api version. I am not aware of what was there in this, as I dont have any change in this , I just copy pasted it to the new version
There was a problem hiding this comment.
yup, thanks for the clarification.
| "type": "object", | ||
| "description": "A resource identity that is managed by the user of the service.", | ||
| "properties": { | ||
| "objectId": { |
There was a problem hiding this comment.
Make sure to use principalId based on our docs
| "readOnly": true, | ||
| "type": "string" | ||
| }, | ||
| "innerError": { |
There was a problem hiding this comment.
Details -> An array of JSON objects that MUST contain name/value pairs for code and message, and MAY contain a name/value pair for target
Inner Error -> The contents of this object are service-defined. Usually this object contains information that will help debug the service. This is generally used in the development environments and not in the production environments. We send null in the production environment
There was a problem hiding this comment.
The problem I see here is that you are making a property to be environment dependent, this is usually modeled as a discriminator with an enum (e.g. environmentType, depending on this type value you can show different information to the user and the user is well aware of it).
If this is for Dev environment, and since this seems to be a single string value, consider modeling this as part of your details property, you can create an error code that identifies that this is a Dev error, this way users can easily identify your error type, otherwise you'll making users to possibly determine their environment based on innerError having a value or not, which is not ideal.
There was a problem hiding this comment.
I will be removing the inner error section and then will discuss among my team how to fix the code.
|
I just reviewed the updated pieces and left some comments. I think the major items are:
Please take a look and reply to the comments. |
|
Hi @jorgecotillo I have responded to the comments and made the required changes |
jorgecotillo
added
ARMSignedOff
|
@jianyexi Can you please see if things are fine to merge this |
No, are you going to merge ? |
|
I cant merge it right. |
|
I mean i dont have the authority to. Thats why i was asking you |
…ervices and s360 fixups (#1797) Create to sync Azure/azure-rest-api-specs#14752 [ReCreate this PR](https://github.com/azure-resource-manager-schemas/compare/master...AzureSDKAutomation:sdkAuto/recoveryservices?expand=1)
…e#14752) * Copied files to new version * Changes in examples for new version * Fixed typo in readme file * Modified api version in json files * Fixed the field name for user assigned identity * Fixed the examples * Fixing other examples * Changed the tag in readme file for sdk generation * Reverted the previous commit * s360 fixups for Microsoft.RecoveryServices * Added word in custom words * Fixed the Semantic error for enum Move State * Changing Object Id back to Principal Id * Changing description in swagger * Modified description and removed Inner error
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with 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
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please 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 there are following updates in the PR, ensure to request an approval from 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.