Update failover group spec. by jaredmoo · Pull Request #1106 · Azure/azure-rest-api-specs

jaredmoo · 2017-04-10T20:14:47Z

Functional differences:

FailoverGroupResource -> FailoverGroup
FailoverGroupResourceList -> FailoverGroupListResult
Defined mutability for properties
Defined pageability for list operation
Added 202 response codes to long running operations

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.

Functional differences: - FailoverGroupResource -> FailoverGroup - FailoverGroupResourceList -> FailoverGroupListResult - Defined mutability for properties - Defined pageability for list operation - Added 202 response codes to long running operations

jaredmoo · 2017-04-10T20:15:23Z

@btasdoven

dsgouda

Added comments

dsgouda · 2017-04-10T21:02:16Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

+            "type": "string"
+          },
+          {
+            "$ref": "#/parameters/ApiVersionParameter"


nit: maintain a consistent parameter ordering, either place ApiVersion and SubscriptionId together at the beginning of the parameters section or at the end.

Consistent with what? This is both consistent within this file and consistent with the order that the parameters appear in the URI.

Consistent with newer specs we are introducing in the repo which we are making sure adhere to this convention. We're in the process of writing a validation rule to that effect.
Simply put we want to ensure that the parameter section has path parameters first followed by body parameters and the client-level parameters (api-version and subscriptionId) in the beginning or at the end of the parameters section.

Ok, this is doable. Do you have a discussion I can link to as justification in my Swagger generator source code?

The closest discussion is this though we haven't articulated the details yet. Hope this helps

dsgouda · 2017-04-10T21:02:40Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

-        },
        "parameters": [
-          {
-            "$ref": "#/parameters/ApiVersionParameter"


nit: maintain a consistent parameter ordering, either place ApiVersion and SubscriptionId together at the beginning of the parameters section or at the end.

dsgouda · 2017-04-10T21:03:22Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

-        },
        "parameters": [
-          {
-            "$ref": "#/parameters/ApiVersionParameter"


nit: maintain a consistent parameter ordering, either place ApiVersion and SubscriptionId together at the beginning of the parameters section or at the end.

dsgouda · 2017-04-10T21:03:45Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

-        },
        "parameters": [
-          {
-            "$ref": "#/parameters/ApiVersionParameter"


nit: maintain a consistent parameter ordering, either place ApiVersion and SubscriptionId together at the beginning of the parameters section or at the end.

dsgouda · 2017-04-10T21:10:18Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

        ],
-        "description": "Sets which server is primary by failing over from the current primary server.",
+        "description": "Fails over from the current primary server to this server.",
        "operationId": "FailoverGroups_Failover",


nit: operationId should be of the noun_verb form do consider changing this but keep in mind that would be a breaking change

"Fail over" is a verb :)

dsgouda · 2017-04-10T21:10:24Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

-        },
        "parameters": [
-          {
-            "$ref": "#/parameters/ApiVersionParameter"


nit: maintain a consistent parameter ordering, either place ApiVersion and SubscriptionId together at the beginning of the parameters section or at the end.

dsgouda · 2017-04-10T21:13:26Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

+      "x-ms-parameter-location": "method"
    },
-    "ServerParameter": {
+    "ServerNameParameter": {


This isn't used anywhere. Either refer it from the appropriate operation parameters or delete it

We will soon be merging sql swagger files into a single file in order to move away from composite swagger, so this parameter will be used by other operations in that combined file. Do you mind if we keep this standard parameter for now?

dsgouda · 2017-04-10T21:15:03Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

    },
-    "FailoverGroupParameter": {
-      "name": "failoverGroupName",
+    "DatabaseNameParameter": {


This isn't used anywhere. Either refer it from the appropriate operation parameters or delete it

dsgouda · 2017-04-11T00:01:47Z

Would also like to point out that this would be a breaking change (since the method signatures are being modified)

Following order specified in Azure#1611

jaredmoo · 2017-04-11T00:12:35Z

Understood there are client breaking changes. Not a major problem since clients haven't been published since this spec was first checked in. Thanks :)

dsgouda · 2017-04-11T00:13:46Z

LGTM pending CI builds

jaredmoo · 2017-04-11T00:15:06Z

Thank you very much for the quick response time @dsgouda !

dsgouda · 2017-04-12T22:31:21Z

@azuresdkci Test this please

dsgouda · 2017-04-13T17:01:05Z

arm-sql/2015-05-01-preview/swagger/failoverGroups.json

      "name": "resourceGroupName",
      "in": "path",
-      "description": "The name of the resource group that contains the resource. You can obtain this value from the Azure Resource Manager API or the portal.",
+      "description": "The name of the resource group that contains the resource.",


I believe the description here does not match with other specs under the same composite swagger, either revert this to the older description or update all the descriptions under the composite swagger to match this. It would break builds otherwise.
The problem here is that when we merge the documents into a single model, its common nodes should be exactly the same, it would be difficult to say which one overrides all other nodes.
A similar issue was filed here

AutorestCI · 2017-04-14T16:41:32Z

No modification for NodeJS

AutorestCI · 2017-04-14T16:42:00Z

Azure/azure-sdk-for-ruby#752

AutorestCI · 2017-04-14T16:42:02Z

Azure/azure-sdk-for-python#1080

jaredmoo · 2017-04-14T16:59:52Z

Thanks @dsgouda !! 👍

* Telephonyservices RP * Update examples * Updated listby examples responses * fix go path * fix go sdk readme

Update failover group spec.

6e5c3be

Functional differences: - FailoverGroupResource -> FailoverGroup - FailoverGroupResourceList -> FailoverGroupListResult - Defined mutability for properties - Defined pageability for list operation - Added 202 response codes to long running operations

msftclas added the cla-not-required label Apr 10, 2017

azuresdkci assigned dsgouda Apr 10, 2017

dsgouda suggested changes Apr 10, 2017

View reviewed changes

Fixed mutability capitalization

8a19310

Reordered params.

91891fd

Following order specified in Azure#1611

dsgouda approved these changes Apr 11, 2017

View reviewed changes

dsgouda reviewed Apr 13, 2017

View reviewed changes

Fixed inconsistent common param names.

35a533c

dsgouda merged commit 85e1845 into Azure:master Apr 14, 2017

AutorestCI mentioned this pull request Apr 14, 2017

Automatic PR from RestAPI-PR1106 Azure/azure-sdk-for-ruby#752

Closed

AutorestCI mentioned this pull request Apr 14, 2017

[SQL] Automatic PR from RestAPI-PR1106 Azure/azure-sdk-for-python#1080

Closed

jaredmoo deleted the failovergroupgen branch July 8, 2017 17:22

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

New RPaaS RP: Microsoft.Telephony (#1106)

a5326e2

* Telephonyservices RP * Update examples * Updated listby examples responses * fix go path * fix go sdk readme

Comments

Conversation

jaredmoo commented Apr 10, 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!

jaredmoo commented Apr 10, 2017

Uh oh!

dsgouda 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!

dsgouda Apr 10, 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!

dsgouda commented Apr 11, 2017

Uh oh!

jaredmoo commented Apr 11, 2017

Uh oh!

dsgouda commented Apr 11, 2017

Uh oh!

jaredmoo commented Apr 11, 2017

Uh oh!

dsgouda commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AutorestCI commented Apr 14, 2017

Uh oh!

AutorestCI commented Apr 14, 2017

Uh oh!

AutorestCI commented Apr 14, 2017

Uh oh!

jaredmoo commented Apr 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jaredmoo commented Apr 10, 2017 •

edited by azuresdkci

Loading

dsgouda Apr 10, 2017 •

edited

Loading