Update ADL for firewall rule support and trusted id provider patch support. by begoldsm · Pull Request #897 · Azure/azure-rest-api-specs

begoldsm · 2017-02-01T00:09:03Z

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.

And add a new property called: firewallAllowAzureIps which is a switch that will also allow Azure originated IP addresses to have connectivity, even if those IPs are not in the FirewallRule list. Remove unused ErrorDetails from ADLA

Will need to add some tests for this since it appears there were no tests for this method.

sarangan12 · 2017-02-02T07:52:56Z

+          }
+        ],
+        "responses": {
+          "200": {


what happens in the case of failure? Don't you want a default response to return some kind of error?

Is this a new requirement? My understanding is that if there is no default response defined, then we use the azure defined default response (see all of our other methods, almost none of them have a default defined).

sarangan12 · 2017-02-02T07:54:08Z

+          }
+        ],
+        "responses": {
+          "200": {


what happens in case of error? Do you want a default error response?

sarangan12 · 2017-02-02T07:57:53Z

+          }
+        ],
+        "responses": {
+          "200": {


You need to include the schema for the response, atlease an operationresponse similar to the one defined in compute.json

I do include a response, this does not return an operation response, it will always either throw an error or return the firewall that it creates. This is not an LRO.

sarangan12 · 2017-02-02T08:00:09Z

+            "description": "Successfully deleted the specified firewall rule"
+          },
+          "204": {
+            "description": "The specified firewall rule does not exist or was already deleted."


I believe you get 204 even when the rule exists. Then the SDKs must poll and eventually will get a 200 result. Am I wrong? If I am not, then the description must be changed to reflect that possibility also.

Again, this is not an LRO, you will only get a 204 if the rule does not exist, you get a 200 if the rule was successfully deleted. This is a direct copy from our existing schema in ADLS.

sarangan12 · 2017-02-02T08:01:15Z

+          }
+        ],
+        "responses": {
+          "200": {


default error response?

sarangan12 · 2017-02-02T08:02:52Z

+          }
+        ],
+        "responses": {
+          "200": {


Default Error response?

sarangan12 · 2017-02-02T08:05:33Z

+        },
+        "firewallAllowAzureIps": {
+          "type": "string",
+          "description": "The current state of allowing or disallowing IPs originating within Azure through the firewall. If the firewall itself is disabled, this is not enforced.",


nitpick: 'itself' is not required

Haha fair, removed :)

sarangan12 · 2017-02-02T08:09:31Z

            "required": true,
            "type": "string",
-            "description": "The name of the Data Lake Store account to which to add the trusted identity provider."
+            "description": "The name of the Data Lake Store account to which to add or replace the trusted identity provider."


This can be rephrased. "to which to add"

I would like to keep the add/replace description, since it will replace the rule if it already exists. I took out the "to which" though, since it adds a lot of extra verbage :)

sarangan12 · 2017-02-02T08:10:20Z

+          }
+        ],
+        "responses": {
+          "200": {


default error response

sarangan12 · 2017-02-02T08:19:04Z

In arm-datalake-analytics/account/2016-11-01/swagger/account.json, Line 418 - Shouldn't the operation be named: StorageAccounts_Create instead of StorageAccounts_Add?

Same comment applies to Line 713 also

sarangan12 · 2017-02-02T08:27:48Z

In https://github.com/begoldsm/azure-rest-api-specs/blob/75b3f28013d093d72d20a03c3ea75ad5bbb46bdf/arm-datalake-store/account/2016-11-01/swagger/account.json:

Line 1058 & Line 1064, Could you please provide justification for the usage of uuid?
Line 1071: Model EncryptionConfig has property type. If possible, can this be renamed? Because, the DataLakeStoreAccount is a tracked resource and it is not recommended to have the properties type, location, tags on it

begoldsm · 2017-02-02T18:18:31Z

@sarangan12 answers to your questions on general swagger design:
Changing encryption config: no, that is a breaking change and this is a published API, so it cannot be removed/changed at the API layer for a very long time. If this is a hard requirement for Swagger (and not just guidance), we could make the breaking change in the SDK by using the x-ms-name attribute, but I would prefer not to

I don't have a justification, other than that it is shipped as a UUID today, and changing it is a breaking change. If it needs to be changed to a string for safety (for read-only properties), we can take an action item to do so during the next breaking change cycle.

begoldsm · 2017-02-02T18:19:03Z

@sarangan12 and in regards to add vs. create, it should be add because the account must already exist, you are simply adding a reference to it underneath the ADLA account.

sarangan12 · 2017-02-02T18:34:03Z

Update:

No Semantic Issues found
Linter Rule has reported on UUID. Synched up with Ben and these changes could be done at the time of breaking change. The rule on add vs create has been explained.
Other comments have been addresses.

Approving this

AutorestCI · 2017-02-02T18:41:42Z

Azure/azure-sdk-for-ruby#666

AutorestCI · 2017-02-02T18:42:03Z

Azure/azure-sdk-for-node#2044

begoldsm added 2 commits January 31, 2017 14:43

Add Firewall rule support to ADLA

75ffdbd

And add a new property called: firewallAllowAzureIps which is a switch that will also allow Azure originated IP addresses to have connectivity, even if those IPs are not in the FirewallRule list. Remove unused ErrorDetails from ADLA

Add patch support for Firewall and Trusted ID.

d1b6c14

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

azuresdkci assigned sarangan12 Feb 1, 2017

begoldsm added 2 commits February 1, 2017 10:01

Fix a path bug in getContentSummary.

6dff23f

Will need to add some tests for this since it appears there were no tests for this method.

Documentation for ip protocols that are supported.

75b3f28

kirthik added the Data Lake Analytics label Feb 2, 2017

sarangan12 suggested changes Feb 2, 2017

View reviewed changes

Address PR comments.

4ba2f9e

sarangan12 approved these changes Feb 2, 2017

View reviewed changes

sarangan12 merged commit 443552a into Azure:master Feb 2, 2017

AutorestCI mentioned this pull request Feb 2, 2017

Automatic PR from RestAPI-PR897 Azure/azure-sdk-for-ruby#666

Merged

AutorestCI mentioned this pull request Feb 2, 2017

Automatic PR from RestAPI-PR897 Azure/azure-sdk-for-node#2044

Closed

begoldsm mentioned this pull request Feb 9, 2017

ADL Preview Release Update Azure/azure-sdk-for-net#2803

Merged

8 tasks

MarcelRaschke mentioned this pull request Aug 20, 2022

[Snyk] Upgrade fs-extra from 3.0.1 to 10.1.0 MarcelRaschke/azure-rest-api-specs#8

Open

Conversation

begoldsm commented Feb 1, 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!

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!

sarangan12 commented Feb 2, 2017

Uh oh!

sarangan12 commented Feb 2, 2017

Uh oh!

begoldsm commented Feb 2, 2017

Uh oh!

begoldsm commented Feb 2, 2017

Uh oh!

sarangan12 commented Feb 2, 2017

Uh oh!

AutorestCI commented Feb 2, 2017

Uh oh!

AutorestCI commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

begoldsm commented Feb 1, 2017 •

edited by azuresdkci

Loading