-
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
vmss: support for passing a health probe to update #7355
Conversation
In Testing, Please Ignore[Logs] (Generated from 5b6736a, Iteration 1).NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]
|
Automation for azure-sdk-for-pythonA 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-goThe initial PR has been merged into your service PR: |
Can one of the admins verify this patch? |
@@ -8435,6 +8435,10 @@ | |||
}, | |||
"VirtualMachineScaleSetUpdateNetworkProfile": { | |||
"properties": { | |||
"healthProbe": { |
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 is a breaking change, please include this in a new api version
See 'New property added to response':
https://microsoft.sharepoint.com/:w:/t/azureresourcemanagerteam/EWXsAQ1yx25KkyYCeeWGUgwBSBxEdUDEbHi6FZ__U8EOQw?e=xEcppo
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.
@ryansbenson this is in the existing API version - it's just missing from the Swagger - please see the examples in the PR description
In addition it appears that link isn't publicaly available:
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.
@ryansbenson since this is already in the API (but is just missing from the Swagger) I'm assuming adding this optional property to an update (to match the API definition) should be fine?
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 @tombuildsstuff for your information, the doc above lists various types of breaking changes, which contains on entry of
New property added to response
If a new property/field is added to the response an API, the GET-PUT pipeline will be broken. Consider the case where from portal a customer updates the value of a new property "A". Another customer does a GET of this resource using the SDK. The SDK will ignore the property since it does not understand it. From the SDK, the customer does a PUT using the model that was returned from the GET. This will overwrite the change made by the first customer from the portal.
Per this entry, the addition of this property is a breaking change to the existing swagger.
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 @ryansbenson per what @tombuildsstuff claims, this property has already existed in the API but just missing in swagger, will this condition needs a new api version?
More to ask, would proposing a new api version be done by customer?
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.
If the live api version accepts and supports this property, it doesn't constitute a breaking api version change (as it already exists in the api version). This is essentially a documentation issue. If these properties came after the api version was released and we're trying to document it now, your customers have already gone through the side affects of the breaking change (this isn't a good experience but not something we can solve in swagger PRs).
Approving from ARMs side
@ArcturusZhang sorry to ping you - would you mind tagging this "Service Team" so the Compute team could take a look? This is unfortunately blocking a couple of new resources we're trying to ship |
The Update struct for a Virtual Machine Scale Set doesn't currently expose the 'HealthProbe' block required when updating a VMSS with an Automatic/Rolling Upgrade Policy using the
Update
method:which currently returns:
This PR adds the
HealthProbe
block to the Update object, which when specified allows the Update to complete successfully:which then returns: