-
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
Azure Spring Cloud: Add location to Apps #8851
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Can one of the admins verify this patch? |
azure-cli-extensions - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-go - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-js - Release
|
azure-sdk-for-python - Release
- Breaking Change detected in SDK
|
azure-sdk-for-net - Release
|
@@ -2043,6 +2043,10 @@ | |||
"$ref": "#/definitions/AppResourceProperties", | |||
"description": "Properties of the App resource", | |||
"x-ms-client-flatten": false | |||
}, | |||
"location": { |
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.
location [](start = 9, length = 8)
Please mark as read-only if it's inherited from parent resource.
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.
location
is needed in user request (PUT/PATCH). I just found making it read-only removes this property from http request in the SDK generated. It might not be a read-only case in this scenario?
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 user puts a location different from parent resource deliberately, a 404 error will be returned due to the uniqueness of parent resource cross all regions
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.
Your create and update examples don't have location in request body. If it's required then it should be marked required in the swagger and in those examples.
The experience around this is kind of weird. We know what the only valid location is and we're going to make the user put it in the request anyway, but if they get it wrong we will reject the request. Why do we make them specify the location at all?
In reply to: 401348345 [](ancestors = 401348345)
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.
Updated the create and update examples. Actually the location is required only when enabling/disabling MSI via PUT/PATCH, so ARM can figure out which regional MSI RP it should call. Otherwise, it's optional.
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
* Azure Spring Cloud: Add location to Apps * make location read-only for Apps * revert: make location read-only for Apps * put location in PUT/PATCH request examples Co-authored-by: Guoqing Geng <[email protected]>
Latest improvements:
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.