[Storage] Supported vnetacl #3347
Conversation
|
swagger PR is already merged, can any one take review on this? |
| /// Defines values for Action. | ||
| /// </summary> | ||
| [JsonConverter(typeof(Newtonsoft.Json.Converters.StringEnumConverter))] | ||
| public enum Action |
There was a problem hiding this comment.
Why do we have an enum with only one value? Seems that this should just be a default value that is always serialized, rether than forcing the user to provide a proeprty that has only one possible value
There was a problem hiding this comment.
"Deny" could be a possible value to be added for disable specific vnetset. That's reason to set it as a enum here.
I already set default value as "Allow" in swagger, https://github.com/Azure/azure-rest-api-specs/blob/master/arm-storage/2017-06-01/swagger/storage.json#L885. But with "x-ms-enum" extension, it seems default value is not set. And "x-ms-enum" is required for swagger's linter check.
There was a problem hiding this comment.
@JasonYang-MSFT Looking at the swagger, you have provided the correct information. My concern is that this property is not required, so that a user doesn't have to provide iot. What happens if this property is not provided by the user? I think you need to explicitly test this.
There was a problem hiding this comment.
I just added a rule without action, this property could be set to "allow" by server side. So in this version, user doesn't need to provide it if user want to create a rule with aciton "allow", though this is the only value.
There was a problem hiding this comment.
Excellent, that is precisely what you swagger spec says, thanks for the extra effort
|
@markcowl Comment is replied. |
| NetworkAcls = new StorageNetworkAcls | ||
| { | ||
| Bypass = @"Logging, Metrics", | ||
| IpRules = new List<IPRule> { new IPRule { IPAddressOrRange = "23.45.67.91", Action = Microsoft.Azure.Management.Storage.Models.Action.Allow } }, |
There was a problem hiding this comment.
This is the potential issue. 'Action' is not a required property for this type. You should have a test that does not supply the Action value
There was a problem hiding this comment.
You could simply add an additional rule to this array that doesn't contain this data
There was a problem hiding this comment.
A rule without action added.
| /// Defines values for Action. | ||
| /// </summary> | ||
| [JsonConverter(typeof(Newtonsoft.Json.Converters.StringEnumConverter))] | ||
| public enum Action |
There was a problem hiding this comment.
@JasonYang-MSFT Looking at the swagger, you have provided the correct information. My concern is that this property is not required, so that a user doesn't have to provide iot. What happens if this property is not provided by the user? I think you need to explicitly test this.
|
@markcowl Comments are addressed. |
|
@azuresdkci retest this please |
Description
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.