-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix S360 Broken Issues][HDInsight]Fix s360 issues batch3 #12612
Conversation
Hi, @aim-for-better 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
Only 10 items are listed, please refer to log for more details.
|
Rule | Message |
---|---|
The child tracked resource, 'azureasyncoperations' with immediate parent 'Cluster', must have a list by immediate parent operation. New: Microsoft.HDInsight/stable/2018-06-01-preview/virtualMachines.json#L132 |
|
Booleans are not descriptive and make them hard to use. Consider using string enums with allowed set of values defined. Property: isDefault New: Microsoft.HDInsight/stable/2018-06-01-preview/locations.json#L209 |
|
Do not have duplicate name of x-ms-example, make sure every x-ms-example name unique. Duplicate x-ms-example: Get the subscription billingSpecs for the specified location New: Microsoft.HDInsight/stable/2018-06-01-preview/locations.json#L161 |
️️✔️
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 @aim-for-better, Your PR has some issues. Please fix the CI sequentially by following the order of
|
NewApiVersionRequired reason: |
Hi @aim-for-better, 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. |
Hi @leni-msft Could you please help review the PR? |
Hi @aim-for-better , pls fix the LintError |
There is a conflict when generating sdk if I fix this linting error. The required error response structure is not consistent with the existing error response in swagger. And the fixing is not the same with the response from our service's backend. What should I do? |
I see. Since this is to fix existing apiVersion, it's ok to me if you fix it in new apiVersion. |
Hi @aim-for-better, is there any SDK refresh this PR change? @msyyc @ArcturusZhang (Python & GO) SDK owner for awareness possible breaking change caused by this fix |
@@ -1733,7 +1790,7 @@ | |||
}, | |||
"description": "Gateway settings." | |||
}, | |||
"OperationResource": { | |||
"AsyncOperationResult": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to rename this model? This renaming causes SDK breaking changes.
This model is not used at all in the previous version of this swagger, therefore could we just keep this name unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to rename this model? This renaming causes SDK breaking changes.
This model is not used at all in the previous version of this swagger, therefore could we just keep this name unchanged?
Hi @ArcturusZhang , Thanks for your comment.
I think the new name is more meaningful. As you said, the class "OperationResource" is never used. I think this breaking change is ok, no customer will be impacted.
I know this PR will cause breaking change but we have to fix S360 issues.I just submitted the sdk release requirement about one week ago. Now we do not have the requirement for release sdk based this PR. Thank you very much~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ArcturusZhang , Is the reply accpetable for you? Besides this class name, there are other changes will cause breaking change too. This PR is fixing the S360 issues which will cause breaking changes in Sdks level.
Hi @akning-ms Thanks for you comment. you concern is right, this PR will actually cause breaking change in Sdks. But now we do not need to refresh any sdks based this PR. I will submit sdk release request if we need. Thank you very much~ |
The reason for the CI"SDK azure-sdk-for-net" Failed is that the sdk test project failed due to the breaking change caused by this PR. |
Hi @leni-msft , We can not fix the DefaultErrorResponseSchema linting error forever in swagger spec due to it will cause breaking change in our service, so I add suppression. Could you please help review again? Thank you very much~ |
* Add networkProperties and clusterId * Add properties: VMGroupName, saskey, fileshare Change vmSizes to vmsizes, vmSizes_filter to vmsizes_filter to fix S360 issue * Suppress R4007 DefaultErrorResponseSchema Co-authored-by: Zhenyu Zhou <zhezhou@microsoft.com>
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
The changes are as bellow:
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.