Add Microsoft.Cache/redis/firewallRules resource operations by TimLovellSmith · Pull Request #977 · Azure/azure-rest-api-specs

TimLovellSmith · 2017-02-28T19:43:31Z

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.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

TimLovellSmith · 2017-02-28T23:15:33Z

Build failed. Looks like TravisCI may have been affected by AWS. Can I get a rebuild?

TimLovellSmith · 2017-03-01T18:39:39Z

Hooray - build passed!

veronicagg

Thanks @TimLovellSmith . I left some comments inline.
Please also take a look at the output of our validation tools:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/206299812#L2515
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/206299814#L2522

veronicagg · 2017-03-01T18:29:35Z

seems that this operation should be marked with x-ms-pageable https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-pageable

@veronicagg Is it required? I don't think we would ever actually page the response in practice.

@TimLovellSmith - https://github.com/Azure/azure-rest-api-specs/pull/977/files#diff-785771b0c72c01e60191a5d798d6d508R1128 The model has a property named nextLink.
If you think that the server will never return more than one server page of response and the service does not have support for nextLink property in the response body then please remove nextLink as the property from the model.

However, you would still need to add the x-ms-pageable extension to the operation. The minor difference would be "nextLinkName": null in the x-ms-pageable extension. This is done explicitly to indicate AutoRest that there is no nextLink. Then AutoRest will still flatten the "value" property and return an IEnumerable<RedisFirewallRule> instead of IPage<RedisFirewallRule>

@amarzavery OK sounds simpler then to just declare x-ms-pageable now in case we ever add it - and it won't matter if server doesn't actually set the nextLink property for now right?

In this case, I think it is ok to fudge. The server always returns the first page. Whenever the server adds the capability to add a next page it wouldn't be a breaking change in the API or the sdk. IMHO, pageable is a standard pattern in all the ARM services. All the services should just copy paste that implementation all the time and make it the defacto standard. In that way there is consistency and the swagger spec is also an accurate representation of the API.

veronicagg · 2017-03-01T19:06:07Z

From https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-checklist.md :
M2017: The model definition for the body parameter and the response MUST be the same for a PUT operation. This ensures that the same entity will be reusable between GET and PUT.
Please evaluate whether the current definition is intentional.

veronicagg · 2017-03-01T19:07:11Z

are there any readOnly properties in this definition?

veronicagg · 2017-03-01T19:08:33Z

is the operation returning the deleted resource for "200"?

@veronicagg Nope, we would just return 200 with no body. Is returning the deleted resource a recommended pattern?

This is what the HTTP spec https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html says:

9.7 DELETE

The DELETE method requests that the origin server delete the resource identified by the Request-URI. This method MAY be overridden by human intervention (or other means) on the origin server. The client cannot be guaranteed that the operation has been carried out, even if the status code returned from the origin server indicates that the action has been completed successfully. However, the server SHOULD NOT indicate success unless, at the time the response is given, it intends to delete the resource or move it to an inaccessible location.

A successful response SHOULD be 200 (OK) if the response includes an entity describing the status, 202 (Accepted) if the action has not yet been enacted, or 204 (No Content) if the action has been enacted but the response does not include an entity.

If the request passes through a cache and the Request-URI identifies one or more currently cached entities, those entries SHOULD be treated as stale. Responses to this method are not cacheable.

veronicagg · 2017-03-01T19:20:16Z

missing the body parameter, which is marked as required in swagger file: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/206299814#L2546

veronicagg · 2017-03-01T21:23:28Z

are you missing flattening for these properties?

veronicagg · 2017-03-01T21:25:36Z

should it be called "ruleName"?

@veronicagg I'm not sure. Is that a recommended pattern for nested resources?

veronicagg · 2017-03-01T22:09:20Z

typo: there's ":" inside the "properties:" string.

TimLovellSmith · 2017-03-02T00:48:54Z

@veronicagg Any idea why linters were thinking that 'RedisCreateParameters' was a tracked resource type?

veronicagg

@TimLovellSmith regarding "RedisCreateParameters" I believe it was wrongly categorized by the tool and we're looking into that.
Are you planning on updating the return on 200 for delete method?
There's one more thing I found, property names should follow camel case format, so some properties "RedisFirewallRuleProperties/startIP", "RedisFirewallRuleProperties/endIP", "RedistProperties/staticIP" and "RedisForceRebootResponseMessage" are being flagged by our validation tools.
For some reason, I don't see CI results for the latest commit, if you push again, I expect to see them.
It looks like the latest update was squashed with the original commit, please try not to do that, as it makes it more difficult to review changes. Thanks!

veronicagg · 2017-03-02T23:03:37Z

+    "subscriptionId": "subid",
+    "parameters": {
+      "properties": {
+        "startIP": "192.168.1.1",


Property names should follow camel case style: "startIp" , similarly for "endIp"

TimLovellSmith · 2017-03-03T20:40:46Z

Closing this and will open new PR, since we've discussed feedbacks, and am not getting new builds...

azurecla added the cla-not-required label Feb 28, 2017

azuresdkci assigned veronicagg Feb 28, 2017

veronicagg suggested changes Mar 1, 2017

View reviewed changes

veronicagg reviewed Mar 1, 2017

View reviewed changes

TimLovellSmith force-pushed the firewallRules3 branch from 9464949 to 0b2f50f Compare March 2, 2017 00:38

TimLovellSmith force-pushed the firewallRules3 branch from 0b2f50f to 0d5f1f1 Compare March 2, 2017 00:52

Add redis firewallRules to swagger specs w/ examples.

6ca78c1

TimLovellSmith force-pushed the firewallRules3 branch from 0d5f1f1 to 6ca78c1 Compare March 2, 2017 00:58

veronicagg suggested changes Mar 2, 2017

View reviewed changes

TimLovellSmith closed this Mar 3, 2017

TimLovellSmith mentioned this pull request Mar 3, 2017

Add redis firewallRules to swagger specs w/ examples. (+minor descrip… #989

Merged

10 tasks

Conversation

TimLovellSmith commented Feb 28, 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!

TimLovellSmith commented Feb 28, 2017

Uh oh!

TimLovellSmith commented Mar 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

veronicagg Mar 1, 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!

veronicagg Mar 1, 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!

TimLovellSmith commented Mar 2, 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!

TimLovellSmith commented Mar 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TimLovellSmith commented Feb 28, 2017 •

edited by azuresdkci

Loading

veronicagg Mar 1, 2017 •

edited

Loading

veronicagg Mar 1, 2017 •

edited

Loading