-
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
Support two apiversion for container service #4237
Conversation
Can one of the admins verify this patch? |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-jsThe initial PR has been merged into your service PR: |
Adding @ravbhatnagar for ARM review of new API version. @zqingqing1 - the changes you made to the readme have removed the OpenShiftManagedClusters spec - was this intentional? |
Automation for azure-sdk-for-goA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
@annatisch , thanks for reminding, it should be included. |
Hi, @annatisch @ravbhatnagar please help to review :) |
@zqingqing1 - please address the model validation errors in the CI:
|
@@ -179,58 +179,6 @@ | |||
} | |||
} | |||
}, | |||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ContainerService/managedClusters/{resourceName}/accessProfiles/{roleName}/listCredential": { |
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.
This appears to be deprecation of an API from a stable version. Do you have a deprecation policy in place? Has this been communicated?
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.
Please see comments on API deprecation
@annatisch @KrisBash error has been address, can you review again? |
@KrisBash , I just saw your comments about api deprecation from email. Not sure why I cannot see the comments here. Anyway, for your question:
No, right now we don't have the deprecation policy. This is a long-term work item, and needs extra discussion with PM team. For now, we just keep it there. |
@annatisch @KrisBash @jhendrixMSFT, I am blocked by this. Can anyone of you help to merge the PR? Thanks in advance. |
@zqingqing1 - the PR name implies that you wish to support two api versions - yet the readme changes do no reflect this. What is your intention here? |
Synced up with @lmazuel before, after this PR gets merged, he will help to support both apiversion for Azure-SDK-for-Python. |
Thanks @zqingqing1 - I see you've removed the property "storageProfile" from ManagedClusterAgentPoolProfile and made "count" a required property. This would be a breaking change in the generated SDKs and would require a major version bump - is this your intention? |
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.
Mostly minor things :)
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Outdated
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Outdated
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Outdated
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Outdated
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Show resolved
Hide resolved
.../resource-manager/Microsoft.ContainerService/preview/2018-08-01-preview/managedClusters.json
Outdated
Show resolved
Hide resolved
…into support-two-apiversion
…rest-api-specs into support-two-apiversion
@annatisch , I changed the tag back to 2018-09-30-preview, so it won't break generated SDK. Meanwhile, After PR merged, @lmazuel can help to support multi api version for SDK-for-python. |
@AutorestCI regenerate azure-sdk-for-go |
Thanks @zqingqing1 - I see be reverting the tag back to 2018-09-30-preview this means that your new spec is excluded from the generated clients for now. Is this to mitigate the breaking changes until you are ready for a major version bump? |
@annatisch , yes. |
Thanks @zqingqing1 - This will result in your new public API not being available in other SDKs. Adding multi-api support only affects Python - whereas reverting the tag will make the public API not available in other languages. If this API is live and public, the SDK generation tags should reflect this and then you can work further with @lmazuel around the specifics of the Python release. |
…rest-api-specs into support-two-apiversion
@AutorestCI regenerate azure-sdk-for-go |
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger