Storage sku list operation and fix a comment to VirtualNetworkResourceId by JasonYang-MSFT · Pull Request #1579 · Azure/azure-rest-api-specs

JasonYang-MSFT · 2017-08-23T15:32:58Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

JasonYang-MSFT · 2017-08-23T15:34:24Z

Also resolve issue #651

azuresdkciprbot · 2017-08-23T15:41:30Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/storage/resource-manager/readme.md
Before the PR: Warning(s): 11 Error(s): 0
After the PR: Warning(s): 12 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

veronicagg

Thanks @JasonYang-MSFT for submitting this PR.
Have these changes been deployed to ARM already?
Since you're adding an operation and some properties, I'm adding @ravbhatnagar from ARM team to take a look and sign-off. Thanks!

veronicagg · 2017-08-24T01:14:08Z

specification/storage/resource-manager/Microsoft.Storage/2017-06-01/storage.json

+          "type": "string",
+          "description": "The name of capability, The capability information in the specified sku, including file encryption, network acls, change notification, etc."
+        },
+        "value": {


should this be an enum?

Do you mean "value" or "SKUCapability"?

@JasonYang-MSFT Maybe I'm off base, but if the "value" an only be "true" or "false", shouldn't be this defined as a boolean directly?

@lmazuel Agreed, too. Boolean is the first thing I tried, but in fact, http response returned a 'string' with value 'true' or 'false' instead of true or false: https://github.com/JasonYang-MSFT/azure-rest-api-specs/blob/storageskus/specification/storage/resource-manager/Microsoft.Storage/2017-06-01/examples/SKUList.json#L69.
It also failed validation with oav validate-example .\storage.json, message: 'Expected type boolean but found type string'. Though generated .Net code could parse it as boolean, but not sure about other languages.

This api exists for a long time, even in 2015-05-01-preview, and it is only in response, so I choose to make it as a string.

Got it. Note that for Python it would have work ok even with the string.

@vishrutshah @amarzavery should ova be improved to accept "string" as potentially valid "bool" input? Or is it really a problem? Seems C# and Python support it at least, and would be a better customer experience.

@lmazuel Isn't the 'true' a json string type and true as json boolean type? I think if service says it's boolean what are the possible reasons for the service to send string 'true'? Doesn't that make swagger contract wrong? I see that point for better SDK experience but not sure we should open gates where swagger contract gets violated? Thoughts?

veronicagg · 2017-08-24T01:17:41Z

specification/storage/resource-manager/Microsoft.Storage/2017-06-01/storage.json

+          },
+          "description": "The capability information in the specified sku, including file encryption, network acls, change notification, etc."
+        },
+        "restrictions": {


considered a breaking change - @ravbhatnagar please review

@veronicagg Do you mean adding parameters to an object? If so, I can create an 'sku' type specially for list operation. Current 'sku' is used in create/update storage account, extra parameters are not used in create/update. Adding extra parameters instead of creating a new object is trying to reuse the type name of 'SKU'.

@veronicagg Just FYI, in Python we consider this non breaking (adding an optional parameter at the end). Please take this opportunity to tell in which language it's breaking (I need to improve my Swagger reviewing skill ;))

@JasonYang-MSFT - ChecknameAvailability API as defined in the ARM RPC should be used to indicate whether a given resource name is available or not. It seems like in this case you are returning more information that what is required in the contract. Why is it needed to return all this extra information like kind, resource type, locations etc. resource type goes in the request body. Locations go in the URL to verify local uniqueness. Not sure why you need kind or SKUCapability to be returned for this API.

@ravbhatnagar Sorry, am I miss something here?
This PR is about sku list operation, "ChecknameAvailability" is not updated in this PR and it is already in the swagger.

Whoops.. @JasonYang-MSFT - please ignore my earlier comment. The way the changes were diff'd in Github gave the impression that all these properties are added to the response of checkNameAvailability :).
The only feedback here is that restrictions should have type, values and reasonCode properties and all these are require for restrictions. restriction.type is most likely locations today. And so the restriction.values will contain the list of locations where the skus is restricted. and then there will be a reason for the restriction in reasoncode. Does that make sense? and yes, this is not a breaking change.

Yes, your comment is addressed. Please kindly take a look.

JasonYang-MSFT · 2017-08-28T14:39:23Z

Hi @veronicagg, what do I need to do to drive this PR forward? It seems we stuck on the discussion. Will @ravbhatnagar evaluate this breaking?

veronicagg · 2017-08-28T17:21:32Z

@JasonYang-MSFT not stuck, we're pending @ravbhatnagar sign-off from ARM side, once that's in, i'm good approving this PR.
Based on what you state below, it appears all these changes are deployed already, right?

JasonYang-MSFT · 2017-08-28T17:47:29Z

@veronicagg Yes, they all deployed already.
Did @ravbhatnagar notice this PR is pending on his sign-off? Do we have a rough ETA?

veronicagg · 2017-08-28T18:15:22Z

@JasonYang-MSFT yes, he should know, plus I sent him offline email (with you cc'd) as well. We have a ton of PRs coming in right now, we're doing our best to keep up.

veronicagg · 2017-08-29T17:22:58Z

@JasonYang-MSFT Please address pending comment from @ravbhatnagar above (#1579 (comment)).
@ravbhatnagar SDK review was done from my side, so when you are ready, please sign-off from ARM side.

azuresdkciprbot · 2017-08-30T02:48:29Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/storage/resource-manager/readme.md
Before the PR: Warning(s): 11 Error(s): 0
After the PR: Warning(s): 12 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

ravbhatnagar · 2017-08-30T23:55:29Z

Looks good. @veronicagg @JasonYang-MSFT ARM signed off.

AutorestCI · 2017-08-31T00:04:07Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-08-31T00:04:21Z

Azure/azure-sdk-for-python#1403

AutorestCI · 2017-08-31T00:04:51Z

Azure/azure-sdk-for-ruby#940

JasonYang-MSFT added 3 commits August 23, 2017 00:35

[Storage] Added support to list skus per subscription.

d0465b1

[Storage] Fixed a comment on virtual network rule.

a25dd94

[Storage] Added example for sku list operaion and fixed an issue.

5a53f7e

msftclas added the cla-not-required label Aug 23, 2017

azuresdkci assigned veronicagg Aug 23, 2017

veronicagg added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 24, 2017

veronicagg suggested changes Aug 24, 2017

View reviewed changes

ravbhatnagar added ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 29, 2017

[Storage] Fixed comments on restriction.

10c4669

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Aug 30, 2017

veronicagg approved these changes Aug 30, 2017

View reviewed changes

veronicagg merged commit 971c9dd into Azure:current Aug 31, 2017

AutorestCI mentioned this pull request Aug 31, 2017

[StorageMgmt] sku list operation + Rename networkAcls to NetworkRuleSet Azure/azure-sdk-for-python#1403

Closed

AutorestCI mentioned this pull request Aug 31, 2017

Automatic PR from RestAPI-PR1579 Azure/azure-sdk-for-ruby#940

Merged

JasonYang-MSFT mentioned this pull request Aug 31, 2017

Microsoft.Azure.Management.Storage 7.0.0 Azure/azure-sdk-for-net#3641

Merged

8 tasks

JasonYang-MSFT mentioned this pull request Sep 8, 2017

azure-arm-storage 3.0.0 Azure/azure-sdk-for-node#2261

Merged

Conversation

JasonYang-MSFT commented Aug 23, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

JasonYang-MSFT commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azuresdkciprbot commented Aug 23, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishrutshah Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonYang-MSFT Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonYang-MSFT commented Aug 28, 2017

Uh oh!

veronicagg commented Aug 28, 2017

Uh oh!

JasonYang-MSFT commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veronicagg commented Aug 28, 2017

Uh oh!

veronicagg commented Aug 29, 2017

Uh oh!

azuresdkciprbot commented Aug 30, 2017

Uh oh!

ravbhatnagar commented Aug 30, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

AutorestCI commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

JasonYang-MSFT commented Aug 23, 2017 •

edited by azuresdkci

Loading

JasonYang-MSFT commented Aug 23, 2017 •

edited

Loading

vishrutshah Aug 24, 2017 •

edited

Loading

JasonYang-MSFT Aug 24, 2017 •

edited

Loading

JasonYang-MSFT commented Aug 28, 2017 •

edited

Loading