Users/tolee/workbooks s360 compliance #11746
Conversation
Swagger Validation Report
Only 10 items are listed, please refer to log for more details.
|
| Rule | Message |
|---|---|
| 'nextLink' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. New: Microsoft.Insights/stable/2020-10-20/myworkbooks_API.json#L385 |
|
| 'nextLink' model/property lacks 'description' and 'title' property. Consider adding a 'description'/'title' element. Accurate description/title is essential for maintaining reference documentation. New: Microsoft.Insights/stable/2020-10-20/workbooks_API.json#L346 |
|
| Do not have duplicate name of x-ms-example, make sure every x-ms-example name unique. Duplicate x-ms-example: WorkbooksList2 New: Microsoft.Insights/stable/2020-10-20/myworkbooks_API.json#L129 |
️️✔️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) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️[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.
Swagger Generation Artifacts
|
|
Hi @tonykslee, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
|
There was an error handling pipeline event 6f3c7b03-576c-4f78-941e-055ddae2b756. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Any tips on getting the model validation to pass without suppression? |
|
Hi, @tonykslee 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] |
| @@ -23,7 +23,7 @@ | |||
| } | |||
| }, | |||
| "responses": { | |||
| "200": { | |||
| "201": { | |||
There was a problem hiding this comment.
201 means Created. For Patch action, it should return 200. So to fix the s360, the service implementation need to be fixed.
There was a problem hiding this comment.
I'm only updating the swagger documentation to reflect the actual response codes that are being returned form this api. Currently upon PATCH request for this api, it returns a 201 and not a 200.
The way I understand it, the cause for the S360 error was because the actual response was returning a 201, but the existing swagger configuration was expecting a 200 (as you are suggesting it should be). Therefore, this PR changes the response code to 201 to match the existing api response code.
There was a problem hiding this comment.
There are quite many breaking changes, as you may have noticed. Per rule made by breaking change review board, you will need follow the process defined in https://aka.ms/AzBreakingChangesPolicy to request breaking approval. Thus, for this specific issue, I would suggest you to change service implementation instead. That will also resolve the S360 issue, and there's no breaking change.
There was a problem hiding this comment.
So it sounds like I understood the S360 error correctly.
there are many breaking changes because the previous swagger definition was defined incorrectly to start with. that is why i'm requesting a breaking change exemption so that we can start regularly maintaining it and keeping it up to date.
It seemed like the full breaking change approval process would take a long time and since the s360 deadline was approaching, I thought that the exception was the right request since these changes are not actually breaking changes - they are correcting the incorrect existing swagger definition. But if there's no way around it and we have to go through the whole process, then that is fine. I'm just concerned that we'd just be out of SLA for the long duration of the approval process.
| @@ -6,7 +6,7 @@ | |||
| "resourceName": "deadb33f-5e0d-4064-8ebb-1a4ed0313eb2" | |||
| }, | |||
| "responses": { | |||
| "201": {}, | |||
| "200": {}, | |||
There was a problem hiding this comment.
Delete should return 200 when done successfully rather than 201. So this is a bugfix. We'd suggest you to ask for approval from breaking change review board.
There was a problem hiding this comment.
Are you suggesting that I add this to the existing swagger kpi exemption request or to create a new one for this change.
There was a problem hiding this comment.
Nope, I suggest you to request breaking change approval for this case. See https://aka.ms/AzBreakingChangesPolicy
| @@ -465,40 +482,47 @@ | |||
| } | |||
| } | |||
| }, | |||
| "ErrorFieldContract": { | |||
| "WorkbookError": { | |||
There was a problem hiding this comment.
it's better to reference definition here: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v2/types.json#L309
There was a problem hiding this comment.
I would like to, but the existing API returns a custom error body that doesn't fit the schema that you referenced.
It includes "innerError" and "trace" as mentioned in the s360 error.
There was a problem hiding this comment.
Is it possible to change the service implementation? You know, to resolve the s360 error, you can either fix swagger or fix service code.
There was a problem hiding this comment.
this is the existing implementation that is already in production in all environments and used by other apis. changing the body here to fit the default error type will cause an actual breaking change. So we don't want to change the service implementation.
| "type": "string", | ||
| "description": "Azure Resource Id that will fetch all linked workbooks.", | ||
| "x-ms-parameter-location": "method" | ||
| }, | ||
| "ResourceIdParameter": { |
There was a problem hiding this comment.
Why are you removing the definition here? Better keep it to avoid breaking change error, if not necessary,
There was a problem hiding this comment.
It doesn't exist and it never has. Apparently the initial definition was incorrect and then still approved. So its been just wrong for many years.
There was a problem hiding this comment.
I see. Then actually it's not a breaking change. You may ask for breaking change approval for this case.
| @@ -23,7 +23,7 @@ | |||
| } | |||
| }, | |||
| "responses": { | |||
| "200": { | |||
| "201": { | |||
There was a problem hiding this comment.
I'm only updating the swagger documentation to reflect the actual response codes that are being returned form this api. Currently upon PATCH request for this api, it returns a 201 and not a 200.
The way I understand it, the cause for the S360 error was because the actual response was returning a 201, but the existing swagger configuration was expecting a 200 (as you are suggesting it should be). Therefore, this PR changes the response code to 201 to match the existing api response code.
| @@ -465,40 +482,47 @@ | |||
| } | |||
| } | |||
| }, | |||
| "ErrorFieldContract": { | |||
| "WorkbookError": { | |||
There was a problem hiding this comment.
I would like to, but the existing API returns a custom error body that doesn't fit the schema that you referenced.
It includes "innerError" and "trace" as mentioned in the s360 error.
| "type": "string", | ||
| "description": "Azure Resource Id that will fetch all linked workbooks.", | ||
| "x-ms-parameter-location": "method" | ||
| }, | ||
| "ResourceIdParameter": { |
There was a problem hiding this comment.
It doesn't exist and it never has. Apparently the initial definition was incorrect and then still approved. So its been just wrong for many years.
| @@ -6,7 +6,7 @@ | |||
| "resourceName": "deadb33f-5e0d-4064-8ebb-1a4ed0313eb2" | |||
| }, | |||
| "responses": { | |||
| "201": {}, | |||
| "200": {}, | |||
There was a problem hiding this comment.
Are you suggesting that I add this to the existing swagger kpi exemption request or to create a new one for this change.
|
Hi, @tonykslee. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
ghost
added
the
JeffreyRichter
added
the
Approved-BreakingChange
|
@kairu-ms Breaking change has been approved. Please merge the PR when you have the chance to review |
@tonykslee Please attach S360 issue links. |
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 API 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.