Add new ChangeAnalysis /resourceChange and /changes API version 2021-04-01 #13285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, @yantang-msft 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. vsswagger@microsoft.com |
Swagger Validation Report
|
| Rule | Message |
|---|---|
| Per the Noun_Verb convention for Operation Ids, the noun 'Changes' should not appear after the underscore. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L111 |
|
| Per the Noun_Verb convention for Operation Ids, the noun 'Changes' should not appear after the underscore. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L162 |
|
| Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L372 |
|
| OperationId should contain the verb: 'resourcechanges' in:'ResourceChanges_List'. Consider updating the operationId Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L63 |
|
| Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isDataMasked Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L325 |
|
| The resource 'Change' does not have get operation, please add it. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L363 |
|
| 'Level' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L271 |
|
| 'changeType' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L287 |
|
| 'level' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L310 |
|
| 'changeType' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. Location: Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json#L351 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️⚠️[Staging] Cross Version BreakingChange (Base on preview version): 9 Warnings warning [Detail]
- Compared Swaggers (Based on Oad v0.8.7)
- original: preview/2020-04-01-preview/changeanalysis.json <---> new: stable/2021-04-01/changeanalysis.json
️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️[Staging] SDK Track2 Validation: 6 Warnings warning [Detail]
- The following tags are being changed in this PR
| Rule | Message |
|---|---|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"The schema 'Resource' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"The schema 'ProxyResource' with an undefined type and 'allOf'/'anyOf'/'oneOf' is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"The schema 'ErrorDetail' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"The schema 'ErrorResponse' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"The schema 'ErrorAdditionalInfo' with an undefined type and decalared properties is a bit ambigious. This has been auto-corrected to 'type:object'" |
|
| "readme":"changeanalysis/resource-manager/readme.md", "tag":"package-2021-04-01", "details":"Checking for duplicate schemas, this could take a (long) while. Run with --verbose for more detail." |
️️✔️[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
Swagger Generation Artifacts
|
|
Hi, @yantang-msft your PR are labelled with WaitForARMFeedback. A notification email will be sent out shortly afterwards to notify ARM review board(armapireview@microsoft.com). cc @ruowan |
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
Hi @yantang-msft, Your PR has some issues. Please fix the CI sequentially by following the order of
|
j5lim
reviewed
Mar 9, 2021
| "level": "Important", | ||
| "description": "Application setting", | ||
| "oldValue": "<old database connection string>", | ||
| "newValue": "<new database connection string>", |
There was a problem hiding this comment.
GET should not return a secret (e.g. connection string)
There was a problem hiding this comment.
@j5lim Is this a hard requirement that GET API should never return secret? Like ARM established the semantics that GET API means there is no secret data?
We set this API as Unauthorized in ARM manifest, and we do the RBAC check ourselves. As the description of the API said, the customer data will be masked if user doesn't have permission, but if they have permission, we will show them the value.
We definitely want to show the value to the customer if they have permission, if it's a hard requirement not to use GET, what do you recommend?
There was a problem hiding this comment.
Switched to POST method.
There was a problem hiding this comment.
Yes, it is a hard requirement. https://armwiki.azurewebsites.net/rp_onboarding/process/property_design_best_practices.html?q=secret#secrets
POST should be used to return secrets.
There was a problem hiding this comment.
@j5lim Thank for the link. I also added the x-ms-secret tag.
Only the resource level query (POST /resourceChanges) may contain secret value, subscription and resource group level query (GET /changes) guarantee the secret values are masked. While in the swagger, they share the same model.
Let me know if you think we should create a different model (e.g., PropertyChangeHasSecret and PropertyChangeNoSecret) just to differentiate the x-ms-secret tag.
There was a problem hiding this comment.
I think GET response won't allow x-ms-secret set to true, so you will need to have a different model.
There was a problem hiding this comment.
@j5lim The Swagger Model validation reports error even for the POST method: https://github.com/Azure/azure-rest-api-specs/pull/13285/checks?check_run_id=2072083599
Is there anything I missed or maybe it's a bug of the validation tool? It's asking me to remove those properties, while we do want to return it.
SECRET_PROPERTY
Output Message: Secret property {0} cannot be sent in the response.
Description: The secret is not allowed to return in response when it's annotated with x-ms-secret:true.
How to fix the violation: Remove this secret value from the response.
Or maybe I misunderstood this tag? i.e., it's meant for the properties that RP never want to return regardless the GET/POST methods? Any in our case since we do want to return them, we shouldn't add this tag? I'll remove this tag for now.
There was a problem hiding this comment.
I talked to Ray Chen, we can ignore the validation error on POST method. To be more specific, I can ping him to get it approved. So I'll add the x-ms-secret tag back and create different models.
There was a problem hiding this comment.
I think it's okay as it is. You can add x-ms-secret for POST but I think it's not necessary.
j5lim
reviewed
Mar 9, 2021
...esource-manager/Microsoft.ChangeAnalysis/stable/2021-04-01/examples/ResourceChangesList.json
Show resolved
Hide resolved
j5lim
reviewed
Mar 9, 2021
...r/Microsoft.ChangeAnalysis/stable/2021-04-01/examples/ChangesListChangesByResourceGroup.json
Outdated
Show resolved
Hide resolved
j5lim
reviewed
Mar 9, 2021
...er/Microsoft.ChangeAnalysis/stable/2021-04-01/examples/ChangesListChangesBySubscription.json
Outdated
Show resolved
Hide resolved
j5lim
reviewed
Mar 9, 2021
...angeanalysis/resource-manager/Microsoft.ChangeAnalysis/stable/2021-04-01/changeanalysis.json
Show resolved
Hide resolved
j5lim
added
ARMChangesRequested
…potentially contains secret value
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
…r if we return this property, no matter if it's GET or POST request, while we very much want to return the values
j5lim
added
ARMSignedOff
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
MirzaSikander
pushed a commit
to MirzaSikander/azure-rest-api-specs
that referenced
this pull request
Mar 22, 2021
…04-01 (Azure#13285) * Copy approved PR in private repo as baseline for new API version * Add new API version 2021-04-01 for ChangeAnalysis * Remove example files that no longer needed since we removd this API in stable version * Remove produce default in swagger * Resolve PR comments * Add x-ms-secret tag on the oldValue and newValue property that could potentially contains secret value * Remove the x-ms-secret tag, the swagger model validation reports error if we return this property, no matter if it's GET or POST request, while we very much want to return the values
mkarmark
pushed a commit
to mkarmark/azure-rest-api-specs
that referenced
this pull request
Jul 21, 2021
…04-01 (Azure#13285) * Copy approved PR in private repo as baseline for new API version * Add new API version 2021-04-01 for ChangeAnalysis * Remove example files that no longer needed since we removd this API in stable version * Remove produce default in swagger * Resolve PR comments * Add x-ms-secret tag on the oldValue and newValue property that could potentially contains secret value * Remove the x-ms-secret tag, the swagger model validation reports error if we return this property, no matter if it's GET or POST request, while we very much want to return the values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.