Add redis firewallRules to swagger specs w/ examples. (+minor descrip… by TimLovellSmith · Pull Request #989 · Azure/azure-rest-api-specs

TimLovellSmith · 2017-03-03T20:43:18Z

…tive changes to existing APIs).
(Reissue of PR #977 to get around CI issues...)

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.

…tive changes to existing APIs).

TimLovellSmith · 2017-03-03T20:44:10Z

(Please assign this to @veronicagg who had #977!)

TimLovellSmith · 2017-03-03T21:38:33Z

@veronicagg It looks like there is some problem with the CI installing/running autorest for node.js?

TimLovellSmith · 2017-03-03T21:45:56Z

@veronicagg
I couldn't figure this message from https://travis-ci.org/Azure/azure-rest-api-specs/jobs/207499990 out

Azure swagger model validation using x-ms-examples and examples in spec /home/travis/build/Azure/azure-rest-api-specs/arm-redis/2016-04-01/swagger/redis.json should have valid examples.:

3360 ReferenceError: error is not defined

3361 at SpecValidator.validateRequest (node_modules/openapi-validation-tools/lib/specValidator.js:567:79)

3362 at SpecValidator.validateXmsExamples (node_modules/openapi-validation-tools/lib/specValidator.js:470:60)

3363 at SpecValidator.validateOperation (node_modules/openapi-validation-tools/lib/specValidator.js:412:10)

3364 at SpecValidator.validateOperations (node_modules/openapi-validation-tools/lib/specValidator.js:446:12)

3365 at node_modules/openapi-validation-tools/validate.js:86:15

TimLovellSmith · 2017-03-03T23:15:55Z

Still getting a cryptic build error after fixing parameter names.
Shouldn't need to be tagged cla-required for me...

amarzavery · 2017-03-04T00:36:13Z

The github CLA bot does not like you :(. It does the same with me many times :(

amarzavery · 2017-03-04T00:41:18Z

        }
      }
    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Cache/Redis/{cacheName}/firewallRules/": {


I think there is an extra slash in the end that shouldn't be there, correct ?
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Cache/Redis/{cacheName}/firewallRules /":

@amarzavery thanks for spotting that!

TimLovellSmith · 2017-03-06T19:38:43Z

@amarzavery What should I do about the CLA issue? Node SDK generation failure is opaque. I also see a lot of linter errors which seem to be extraneous to the content of the specific PR, I think they are caused by improvements to the linter - are any of those going to be blockers for this?

TimLovellSmith · 2017-03-07T17:35:42Z

@veronicagg any advice on how to deal with the linter errors here?

veronicagg

@TimLovellSmith I made some comments inline related to linter errors. And here are some comments for the others:
Is there an operations API you can expose for the service? This error https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L979 comes from a request from the ARM team to have an operation in the spec that lists all the available operations, like: https://github.com/Azure/azure-rest-api-specs/blob/master/arm-cdn/2016-10-02/swagger/cdn.json#L1578

I'm following up on errors with code "ArmResourcePropertiesBag", as I believe the rule may not be appropriate for those cases. (https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L1001)

Even though, this issue is not caused by this PR, please take a look at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L1090

veronicagg · 2017-03-07T19:19:45Z

+          "Redis",
+          "FirewallRules"
+        ],
+        "operationId": "RedisFirewallRules_List",


Is there a reason why the operation is named RedisFirewallRules_List? To follow the convention, based on our latest rules: "Operation RedisFirewallRules_List must be one of: firewallrules_listbysubscriptionid, firewallrules_list, firewallrules_listbyresourcegroup, firewallrules_listbyredis" (see ahttps://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L968), naming this operation "FirewallRules_List" would fix the issue.

@veronicagg The naming guidance seems a little arbitrary. E.g. We call the create operation RedisFirewallRule_CreateOrUpdate and there's no error for that? (Or I overlooked it)

veronicagg · 2017-03-07T19:24:37Z

+        }
+      }
+    },
    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Cache/Redis/{name}/patchSchedules/default": {


https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L990
The recommendation would be to follow the pattern /typename1/{typename1type}/typename2/{typename2type} after the namespace in the path. In this case, it's complaining because of "default" at the end. This could be resolved by replacing "default" with a {property} and have the property as part of the parameters, you could define it as an enum with one value "default" which would produce a similar result.

TimLovellSmith · 2017-03-07T20:27:14Z

@veronicagg What contexts will humans be exposed to the operation name FirewallRules_List/firewallrulesListByRedis in? Do operation names have to be globally unique for any reason (e.g. using as programmatic ID)? The latter is not an intuitive name, so I'd prefer FirewallRules_List if possible, I just want to make sure there's no extra constraints...

TimLovellSmith · 2017-03-07T20:48:42Z

@veronicagg I've made the change with patchSchedules 'default' as enum - I do still have a worry here that this might have been overvalidation - as it might end up making the actual generated sdk client more confusing to have to specify an enumeration with a single value... but we can wait to preview what sdk changes would result.

TimLovellSmith · 2017-03-07T20:56:22Z

@veronicagg
Re: I've made the change with patchSchedules 'default' as enum" - now it seems to just fail model validation even worse... I think I made the required changes to examples here though.

{ code: 'REQUEST_VALIDATION_ERROR',
  message: 'Found errors in validating the request for x-ms-example "RedisCachePatchSchedulesGet" in operation "PatchSchedules_Get".',
  innerErrors: 
   [ { code: 'REQUIRED_PARAMETER_EXAMPLE_NOT_FOUND',
       message: 'In operation "PatchSchedules_Get", parameter name is required in the swagger spec but is not present in the provided example parameter values.' } ] }
{ AssertionError: swagger "arm-redis/2016-04-01/swagger/redis.json" contains model validation errors.
    at /home/travis/build/Azure/azure-rest-api-specs/test/model.js:21:14
  name: 'AssertionError',
  actual: false,
  expected: true,
  operator: '==',
  message: 'swagger "arm-redis/2016-04-01/swagger/redis.json" contains model validation errors.',
  generatedMessage: false }
    1) arm-redis/2016-04-01/swagger/redis.json should have valid examples.

TimLovellSmith · 2017-03-07T21:01:58Z

@veronicagg No, my bad, I didn't update parameter set for all METHODs.

veronicagg · 2017-03-08T18:56:45Z

@TimLovellSmith for your fist question "FirewallRules_List" looks good, the operation id is used in the method name in the SDK.
Regarding the parameter/enum question, see https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/swagger-extensions.md#single-value-enum-as-a-constant

…ist' for what we hope will be nicer method naming.

TimLovellSmith · 2017-03-08T19:13:58Z

@veronicagg Thanks for the answers! I'll change it to FirewallRules_List then, and bring back the patchSchedules/default change.

TimLovellSmith · 2017-03-08T20:53:18Z

@veronicagg So I think I've fixed all the important feedbacks and linter errors. However, node SDK build job still seems to be failing in TravisCI - and outputting usage instructions instead of an actual error!? Could you please check on that?

veronicagg · 2017-03-09T18:21:06Z

@TimLovellSmith we're working on fixing the CI failure that you're seeing, we expect to have that working today.
Changes look good, are you planning on fixing this one too?

Is there an "operations API" you can expose for the service? This error https://travis-ci.org/Azure/azure-rest-api-specs/jobs/208317608#L979 comes from a request from the ARM team to have an operation in the spec that lists all the available operations, like: https://github.com/Azure/azure-rest-api-specs/blob/master/arm-cdn/2016-10-02/swagger/cdn.json#L1578

Thanks!

TimLovellSmith · 2017-03-09T18:40:33Z

@veronicagg No, I was not planning to address that as part of this PR. Is there an easy way to define the operations API without having to define the entire operation operations body shape in our redis.json? It's rather a lot of declarations to add, and I would think usually every RP implements the same contract.

veronicagg · 2017-03-11T01:37:32Z

@TimLovellSmith There seem to still be issues with CI, not just yours, so I've pinged someone else on that. I'll restart this job in case it helps anyway.
Regarding the OperationAPI: does the way that CDN defines it not work well for yours? What type of "easier" way would you be looking for in this case?
One more comment, after chatting with @ravbhatnagar a bit more, our validation for ProvidersPathValidation is a bit too strict and defining a parameter as a constant is not required, so you can leave it as is, or revert back to having the value in the path at https://github.com/Azure/azure-rest-api-specs/pull/989/files#diff-785771b0c72c01e60191a5d798d6d508R755; we'll be updating our validation tool to account for this soon.

TimLovellSmith · 2017-03-13T23:22:51Z

@veronicagg Re "Operations" api, I think I was hoping for something like "Resource" such that every RP definition can link to one common definition, since they should all follow the same basic contract. Although yes, "Resource" is in our spec too...

veronicagg · 2017-03-14T00:57:31Z

@TimLovellSmith ah, yes, I believe you're asking for https://github.com/Azure/azure-rest-api-specs-pr/pull/40, it's coming, at which point you'll be able to point to the common definition.

TimLovellSmith · 2017-03-14T16:03:29Z

@veronicagg I had a look at pull/40 but I don't see Operations anywhere in that diff? It feels nitpicky that we are discussing linter errors Operations API as part of this PR as it is really orthogonal to the changes proposed. Edit... I suck at reading swagger

TimLovellSmith · 2017-03-14T16:41:10Z

@veronicagg I've added the Operations API... its definition slightly varies from CDN's.

veronicagg

Minor comment regarding descriptions. I agree this change is orthogonal with the proposed change, so in my previous comments I was mostly asking if you were planning on addressing it now or not. What's blocking this PR at the moment, is that we're having issues with CI, I've restarted the job a couple of times, there seems to still be failures, which I'm trying to understand.

veronicagg · 2017-03-14T16:52:05Z

+      }
+    },
+    "OperationListResult": {
+     "description": "Result of the request to list CDN operations. It contains a list of operations and a URL link to get the next set of results.",


please update description

veronicagg · 2017-03-14T16:52:14Z

+          "items": {
+            "$ref": "#/definitions/Operation"
+          },
+          "description": "List of CDN operations supported by the CDN resource provider."


please update description

veronicagg · 2017-03-14T17:41:31Z

@ravbhatnagar do you have any thoughts on the question above regarding OperationsAPI?: "@veronicagg I had a look at pull/40 but I don't see Operations anywhere in that diff? It feels nitpicky that we are discussing linter errors Operations API as part of this PR as it is really orthogonal to the changes proposed. Edit... I suck at reading swagger" thanks!

TimLovellSmith · 2017-03-14T22:16:28Z

@veronicagg Woohoo, checks passed!

AutorestCI · 2017-03-14T23:11:15Z

Azure/azure-sdk-for-ruby#717

AutorestCI · 2017-03-14T23:11:23Z

Azure/azure-sdk-for-python#1033

AutorestCI · 2017-03-14T23:11:27Z

Azure/azure-sdk-for-node#2101

TimLovellSmith · 2017-03-15T15:25:46Z

@veronicagg Thank you!

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

d26ed66

…tive changes to existing APIs).

msftclas added the cla-required label Mar 3, 2017

azuresdkci assigned veronicagg Mar 3, 2017

redis.json: fix parameter name mismatch

26665ef

amarzavery reviewed Mar 4, 2017

View reviewed changes

redis.json: fix trailing slash for list 'firewallRules/'

44385c9

hovsepm added cla-not-required and removed cla-required labels Mar 6, 2017

veronicagg suggested changes Mar 7, 2017

View reviewed changes

TimLovellSmith force-pushed the firewallRules3 branch from d6e3d16 to 38ee3a2 Compare March 8, 2017 17:04

Change operationId from 'RedisFirewallRules_List' to 'FirewallRules_L…

ad66be2

…ist' for what we hope will be nicer method naming.

TimLovellSmith force-pushed the firewallRules3 branch from 38ee3a2 to ad66be2 Compare March 8, 2017 19:12

redis.json: declare the GET Microsoft.Cache/operations API

6500525

TimLovellSmith force-pushed the firewallRules3 branch from 2e16c46 to 6500525 Compare March 14, 2017 16:40

veronicagg suggested changes Mar 14, 2017

View reviewed changes

redis.json: fix mentions of CDN

6413c6a

veronicagg approved these changes Mar 14, 2017

View reviewed changes

veronicagg merged commit 65fa4a6 into Azure:master Mar 14, 2017

AutorestCI mentioned this pull request Mar 14, 2017

Automatic PR from RestAPI-PR989 Azure/azure-sdk-for-ruby#717

Merged

AutorestCI mentioned this pull request Mar 14, 2017

[Redis] Fix issues + new Autorest Azure/azure-sdk-for-python#1033

Merged

AutorestCI mentioned this pull request Mar 14, 2017

Automatic PR from RestAPI-PR989 Azure/azure-sdk-for-node#2101

Closed

Conversation

TimLovellSmith commented Mar 3, 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 Mar 3, 2017

Uh oh!

TimLovellSmith commented Mar 3, 2017

Uh oh!

TimLovellSmith commented Mar 3, 2017

Uh oh!

TimLovellSmith commented Mar 3, 2017

Uh oh!

amarzavery commented Mar 4, 2017

Uh oh!

amarzavery Mar 4, 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!

TimLovellSmith Mar 6, 2017

Choose a reason for hiding this comment

Uh oh!

TimLovellSmith commented Mar 6, 2017

Uh oh!

TimLovellSmith commented Mar 7, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

veronicagg Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

TimLovellSmith Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

veronicagg Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

TimLovellSmith commented Mar 7, 2017

Uh oh!

TimLovellSmith commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimLovellSmith commented Mar 7, 2017

Uh oh!

TimLovellSmith commented Mar 7, 2017

Uh oh!

veronicagg commented Mar 8, 2017

Uh oh!

TimLovellSmith commented Mar 8, 2017

Uh oh!

TimLovellSmith commented Mar 8, 2017

Uh oh!

veronicagg commented Mar 9, 2017

Uh oh!

TimLovellSmith commented Mar 9, 2017

Uh oh!

veronicagg commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimLovellSmith commented Mar 13, 2017

Uh oh!

veronicagg commented Mar 14, 2017

Uh oh!

TimLovellSmith commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimLovellSmith commented Mar 14, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

veronicagg Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

veronicagg Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Mar 14, 2017

TimLovellSmith commented Mar 3, 2017 •

edited by azuresdkci

Loading

amarzavery Mar 4, 2017 •

edited

Loading

TimLovellSmith commented Mar 7, 2017 •

edited

Loading

veronicagg commented Mar 11, 2017 •

edited

Loading

TimLovellSmith commented Mar 14, 2017 •

edited

Loading