-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Compute Swagger spec update :- Add the missing PATCH for VmssExtension operation #7724
Conversation
Can one of the admins verify this patch? |
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-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: |
…t may be required (with readonly for type)
specification/compute/resource-manager/Microsoft.Compute/stable/2019-07-01/compute.json
Outdated
Show resolved
Hide resolved
specification/compute/resource-manager/Microsoft.Compute/stable/2019-07-01/compute.json
Show resolved
Hide resolved
specification/compute/resource-manager/Microsoft.Compute/stable/2019-07-01/compute.json
Show resolved
Hide resolved
LGTM |
} | ||
} | ||
}, | ||
"x-ms-long-running-operation": true |
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.
As this is a long running operation, is it better to return 202 instead of 200?
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.
I see that for all the other existing resources (VM, VMSS, AvSet, Image, VMExtension..) as well, only 200 (OK) is specified in the spec.
But from the code, and as CRP treats "patch" & "put" the same way -- added 201 (Created) as well.
@hyonholee to help confirm as well.
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.
Signing off from ARM side barring the one feedback which is to consider adding 202 or 201 (with non-terminal provisioning state) to indicate its long running operation
@srinath9795, please let me know if the code is ready to merge. |
@lirenhe - Yes, the changes for this PR are ready to be merged. Thanks! |
…n operation (Azure#7724) * Swagger spec update :- Add the missing PATCH for VmssExtension operation * remove name * another try by adding back name with "readonly". I guess, a 3rd commit may be required (with readonly for type) * Added "readOnly": true for type * add type explicitly in VMSSExtension & Vmss Extension Update. * Make the publisher & type "readonly" in the VirtualMachineScaleSetExtensionUpdateProperties * just use VirtualMachineScaleSetExtensionProperties * Add 201 as well.
Hi @lirenhe @ravbhatnagar, this PR introduced a breaking change for AKS. See Azure/aks-engine#2605 for details. TLDR: we were relying on Go struct embedding of |
@jhendrixMSFT, have you seen this issue? It's a pretty gnarly one for Golang in particular. |
@devigned thanks for bringing this up. It highlights how model flattening is not version-resilient. FYI I've been considering removing this anonymous embedding in track 2, I think this example strengthens the case. |
Latest improvements:
Added the missing PATCH support for the VMSSExtension operation in the Compute Swagger spec.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.