Skip to content
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

Merged
merged 3 commits into from
May 1, 2019

Conversation

solankisamir
Copy link
Member

@solankisamir solankisamir commented May 1, 2019

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

@AutorestCI
Copy link

AutorestCI commented May 1, 2019

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3106

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented May 1, 2019

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@solankisamir
Copy link
Member Author

Fix for user raised issue in the spec
Azure/azure-sdk-for-node#4718

@AutorestCI
Copy link

AutorestCI commented May 1, 2019

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented May 1, 2019

Automation for azure-sdk-for-js

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-js#2618

@AutorestCI
Copy link

AutorestCI commented May 1, 2019

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#4723

@solankisamir
Copy link
Member Author

solankisamir commented May 1, 2019

@veronicagg this looks like an issue with the rest-api-specs validator. The property properties.document is a jobject here. The validator is treating it like another definition and trying to figure out a file.

I can remove the example, but it would be good to have it fixed.

@solankisamir
Copy link
Member Author

@veronicagg a gentle ping on this. This is customer reported issue and we are trying to fix it. The spec was incorrect.

Copy link
Contributor

@veronicagg veronicagg left a 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"
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member Author

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.

@veronicagg veronicagg merged commit 8762361 into Azure:master May 1, 2019
mccleanp pushed a commit that referenced this pull request Mar 23, 2022
* Copying old version 2021-09-01-preview as base

* Added nodepools API

* Prettier and spell check fix

* added readonly for agentpool status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants