Skip to content
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

[Storage] Upgrade API version to 2020-08-01-preview, and version to 18.0.0-beta #16355

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

blueww
Copy link
Member

@blueww blueww commented Oct 28, 2020

@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label Oct 28, 2020
@blueww blueww changed the title [Storage] Upgrade API version to 2020-08-01-preview, and version to 18.0.0-preview [Storage] Upgrade API version to 2020-08-01-preview, and version to 18.0.0-beta Oct 29, 2020
Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Wei,

Can you include a link to the swagger PR for this change?

Otherwise, this looks good.

@blueww
Copy link
Member Author

blueww commented Nov 2, 2020

@markcowl
Thanks for the review!

I have regenerated the SDK code from latest swagger ,and add a new commit into this PR, to support Blob inventory.

The related swagger swagger PRs are:
Azure/azure-rest-api-specs#10570
Azure/azure-rest-api-specs#10868
Azure/azure-rest-api-specs#10900
Azure/azure-rest-api-specs#10993
Azure/azure-rest-api-specs#11048

@@ -10,7 +10,7 @@
Development of this library has shifted focus to the Azure Unified SDK. The future development will be focused on "Azure.ResourceManager.Storage" (https://www.nuget.org/packages/Azure.ResourceManager.Storage/). Please see the package changelog for more information.
</Description>
<AssemblyName>Microsoft.Azure.Management.Storage</AssemblyName>
<Version>17.2.0</Version>
<Version>18.0.0-beta</Version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 18.0.0-beta.1 to conform with the versioning guidelines: https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-versionnumbers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.


**Breaking changes**

- In StorageManagementClient.FileServices.SetServiceProperties(), add a madatory parameter with type Microsoft.Azure.Management.Storage.Models.FileServiceProperties, to input all FileService properties, and remove 2 parameters to input FileService properties: cors, shareDeleteRetentionPolicy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a temporary change for the preview, or do you intend to go through with this change in the stable API? This will need to go through a breaking change review before you can release as a stable api

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaking will also in SDK of stable API version. How to go through a breaking change review?
I would suggest to merge this PR then start the process to breaking review (or resolve the breaking).

Actually, this is not caused by the swagger (or API) breaking, but caused by a limitation of autorest.

We have config payload-flattening-threshold: 2 in https://github.com/Azure/azure-rest-api-specs/blob/master/specification/storage/resource-manager/readme.csharp.md#common-c-settings, so when a object has <= 2 child properties, in autorest generated code, functions will use the child properties as input in stead of the object; when the object has >=3 properties, functions will use the object as input.

So when FileServiceProperties properties count increase from 2 to 3, the function SetServiceProperties() will change from input the file service properties to input FileServiceProperties object.

This kind of breaking happens frequently in .net SDK. Do you have any idea how to resolve this?
If we change the payload-flattening-threshold: 2 to a bigger value, it will also cause many of the existing API breaking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can likely solve it in individual cases with customizations. Can you send me an email, so I can bring in some generator folks?

@blueww
Copy link
Member Author

blueww commented Nov 9, 2020

@markcowl
Would you please help to review again?
I have fixed/replied all your comments.

PSH new feature depends on this PR, hope we can we merge it ASAP?

@markcowl markcowl merged commit ee576ed into Azure:master Nov 10, 2020
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
…8.0.0-beta (Azure#16355)

* [Storage] Upgrade to 2020-08-01-preview

* [Storage] Support blob Inventory rule

* [storage] Update version to 18.0.0-beta.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants