-
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
[SRP] Add new property EnabledProtocols for file shares #7815
Conversation
Approved in private branch.
Automation for azure-sdk-for-goA PR has been created for you: |
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: |
}, | ||
"description": "Protocols for file shares" | ||
}, | ||
"rootSquash": { |
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.
The enum name is same as one of it's value (both "RootSquash"). This will make the .NET SDK code generated from it build fail.
@@ -239,7 +242,7 @@ | |||
"in": "body", | |||
"required": true, | |||
"schema": { | |||
"$ref": "#/definitions/FileShare" | |||
"$ref": "#/definitions/FileShareCreateParameters" |
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.
Why are you forking the body definition for create/put? With the readonly fields of FileShare, FileShareCreateParameters is effectively the same. Doing this will generate a 2 classes the user would interact with for no real reason.
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.
EnabledProtocol is the property that can only be set at creation. We can stay with one body definition. But customer will be confused since enabledProtocol cannot be updated.
@@ -524,10 +544,107 @@ | |||
"minimum": 1, | |||
"maximum": 5120, | |||
"description": "The maximum size of the share, in gigabytes. Must be greater than 0, and less than or equal to 5TB (5120)." | |||
}, | |||
"enabledProtocols": { |
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.
Adding writable fields to a stable api version is seen as a breaking change and requires a new api version. See the section labeled 'New property added to response' in the following link. This was introduced long back to prevent breaking previous versions of the sdk.
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.
From the server side, if the new properties did not show up in the request, we will no wipe them out. We have a lot of experience in adding new properties to storage account in the existing API version. It (Get-Put model) is not a concern from my prospect..
@@ -312,8 +315,7 @@ | |||
"200": { | |||
"description": "OK -- Update Share operation completed successfully.", | |||
"schema": { | |||
"$ref": "#/definitions/FileShare", | |||
"description": "Properties of the updated file share." |
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.
We should not remove the description, or the doc/SDK generated from swagger will missing description for this property.
We can change it to make it more accurate.
The feature target public preview on 2020,Feb , so don't merge it now. |
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 for ARM. Issues with enum name notwithstanding
Comment was made before the most recent commit for PR 7815 in repo Azure/azure-rest-api-specs |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
azure-sdk-for-go
|
azure-sdk-for-net
|
azure-sdk-for-js
|
azure-sdk-for-java
|
azure-sdk-for-python
|
"name": "EnabledProtocols", | ||
"modelAsString": true | ||
}, | ||
"description": "Immutable property for file shares protocol.", |
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 add more description for what the value means. What's the effect when specify each of the 2 values. What's the default value.
"name": "RootSquashType", | ||
"modelAsString": true | ||
}, | ||
"description": "Reduction of the access rights for the remote superuser." |
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 add more description for What's the effect when specify each of the 3 values. What's the default value.
Approved in private branch. https://github.com/Azure/azure-rest-api-specs-pr/pull/859
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.