-
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
[Hub Generated] Review request for Microsoft.ApiManagement to add version stable/2019-01-01 #5824
Conversation
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Can one of the admins verify this patch? |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
6e647cf
to
7b796fc
Compare
Fix for user raised issue in the spec |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Automation for azure-sdk-for-jsThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
@veronicagg this looks like an issue with the rest-api-specs validator. The property I can remove the example, but it would be good to have it fixed. |
7b796fc
to
fc2b834
Compare
@veronicagg a gentle ping on this. This is customer reported issue and we are trying to fix it. The spec was incorrect. |
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.
question inline.
@@ -2296,7 +2296,7 @@ | |||
"in": "body", | |||
"required": true, | |||
"schema": { | |||
"$ref": "./definitions.json#/definitions/SchemaContract" | |||
"$ref": "./definitions.json#/definitions/SchemaCreateOrUpdateContract" |
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.
Updating the name here may cause a breaking change in the SDKs since the name of the type representing this definition is changing. Do you need to update this one? or should the update to "document" property be happening in the SchemaContract definition?
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.
For CRUD on Schema
, the CreateOrUpdate contract different from the response SchemaContract
for CreateOrUpdate the contract requires properties.document.value
but the response is properties.document
In existing clients this scenario was broken, hence we need to split the contract, otherwise, when the resource is created client cannot serialize the document
in the response
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.
we can make document
as jobject
, but it will degrade the experience when writing clients. Since the scenario anyway was not working, I think creating a good client experience is fine.
* Copying old version 2021-09-01-preview as base * Added nodepools API * Prettier and spell check fix * added readonly for agentpool status
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: