Swagger change for getting all availability sets in a subscription and StandardSSD #3302

dochung4 · 2018-06-25T21:10:04Z

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.

AutorestCI · 2018-06-25T21:11:55Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2710

AutorestCI · 2018-06-25T21:12:57Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2487

AutorestCI · 2018-06-25T21:13:06Z

Automation for azure-sdk-for-go

Encountered a Subprocess error: (azure-sdk-for-go)

Command: ['/usr/local/bin/autorest', '/tmp/tmp2k67_1nf/rest/specification/compute/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmp2k67_1nf/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '--use=@microsoft.azure/autorest.go@~2.1.105', '--use-onever', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
   Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.105->2.1.105)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2018-04"} .
FATAL: System.InvalidOperationException: Swagger document contains two or more x-ms-enum extensions with the same name 'StorageAccountTypes' and different values: Standard_LRS,Premium_LRS vs. Standard_LRS,Premium_LRS,StandardSSD_LRS
   at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\ObjectBuilder.cs:line 147
   at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SchemaBuilder.cs:line 204
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SchemaBuilder.cs:line 46
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SchemaBuilder.cs:line 131
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SchemaBuilder.cs:line 131
   at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SwaggerModeler.cs:line 271
   at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\SwaggerModeler.cs:line 66
   at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in C:\Users\ci\AppData\Local\Temp\PUBLISH8kiit\38_20171116T010915\autorest.modeler\src\Program.cs:line 60
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at NewPlugin.<Process>d__15.MoveNext()
FATAL: go/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
Failure during batch task - {"tag":"package-2018-04"} -- Error: Plugin imodeler1 reported failure..
  Error: Plugin imodeler1 reported failure.

AutorestCI · 2018-06-25T21:21:50Z

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2165

AutorestCI · 2018-06-25T21:23:08Z

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1422

lmazuel · 2018-06-25T22:52:07Z

specification/compute/resource-manager/Microsoft.Compute/stable/2018-04-01/disk.json

@@ -783,7 +783,8 @@
          "type": "string",
          "enum": [
            "Standard_LRS",
-            "Premium_LRS"
+            "Premium_LRS",
+            "StandardSSD_LRS"


This creates invaliad Swagger:
FATAL: System.InvalidOperationException: Swagger document contains two or more x-ms-enum extensions with the same name 'StorageAccountTypes' and different values: Standard_LRS,Premium_LRS vs. Standard_LRS,Premium_LRS,StandardSSD_LRS

I forgot to update 2018-04-01 compute.json. It is updated now.

lmazuel · 2018-06-25T22:54:22Z

...ification/compute/resource-manager/Microsoft.Compute/preview/2016-04-30-preview/compute.json

+        "tags": [
+          "AvailabilitySets"
+        ],
+        "operationId": "AvailabilitySets_ListAll",


Recommendation is ListBySubscription

…ated name to ListAll, and removed the change from 2015-06-15

lmazuel · 2018-06-28T15:45:54Z

...ification/compute/resource-manager/Microsoft.Compute/preview/2016-04-30-preview/compute.json

+          }
+        },
+        "x-ms-pageable": {
+          "nextLinkName": "nextLink"


The provided example creates a linter error:

The property 'nextLink' specified by nextLinkName does not exist in the 200 response schema. \nPlease, specify the name of the property that provides the nextLink. If the model does not have the nextLink property then specify null.

Could you either:

Update the example to include an example of nextLink if server supports it

Update the Swagger to replace "nextLink" by null if server does not support nextLink

Whatever you do, do NOT remove "x-ms-pageable", that's the right thing to do since it's a method that has the semantic of a list.

Can you please check again? I think the issue is not that nextLink is missing in the example, but that nextLink is missing AvailabilitySetListResult in compute.json.

Ok, I see what you mean, you re-used AvailabilitySetListResult as it was defined. Let me check what should be the correct way to remove this warning then.

@dochung4 I made an actual call to the endpoint, and didn't get a nextLink back in the JSON. But maybe I didn't have enough resource to trigger the nextLink. But if it's the truth and the endpoint always returns a unique page whatever the number of AS, then the correct definition is to use null.
Could you confirm the actual server behavior:

If server never returns nextLink, use null

If server may return nextLink, use what you did nextLink

Swagger must described accurately the server, and since I don't own the server implementation I don't know which one is the truth. Your last commit changed a lot of null to nextLink and that concerns me a bit.

For which call did you not get a nextLink back? Currently we have nextLink for listing all availability sets in a subscription, but not for listing all resource groups in a resource group. But I discussed this with my team and I added nextLink to the latter too, because it will be enabled very soon.

Please let me know if you have other thoughts.

I checked listing AS in a RG indeed. So if I understand you correctly:

List AS at the subscription level always returns nextLink

List AS at the RG level didn't have nextLink, but you're adding it live on all api-versions from 2016-04-30-preview to now soon.

Is this accurate?
If this is accurate, I'm good with the PR.

Yes that's correct.

lmazuel · 2018-06-29T22:35:36Z

@dochung4 LGTM.
However, I realized before merging that Compute 2018-06-01 has already a spec, and your change would not be inside. Please update 2018-06-01 as well and I merge it.

dochung4 · 2018-06-29T23:34:34Z

@lmazuel Regarding 2018-06-01, I discussed with my manager, and we agreed to not update 2018-06-01 at this point and let the next person working on Swagger change to make the change.

dochung4 added 2 commits June 25, 2018 14:05

First commit

474690a

Small change

c60eb5a

azuresdkci assigned lmazuel Jun 25, 2018

azuresdkci requested a review from lmazuel June 25, 2018 21:11

AutorestCI mentioned this pull request Jun 25, 2018

[AutoPR compute/resource-manager] Swagger change for getting all availability sets in a subscription and StandardSSD Azure/azure-sdk-for-java#2141

Merged

AutorestCI mentioned this pull request Jun 25, 2018

[AutoPR compute/resource-manager] Swagger change for getting all availability sets in a subscription and StandardSSD Azure/azure-sdk-for-ruby#1421

Merged

Added an example for listing all the availability sets in a subscription

8ba6300

lmazuel requested changes Jun 25, 2018

View reviewed changes

lmazuel reviewed Jun 25, 2018

View reviewed changes

Addressed comments - added missing StandardSSD_LRS in 2018-04-01, upd…

41175e4

…ated name to ListAll, and removed the change from 2015-06-15

AutorestCI mentioned this pull request Jun 28, 2018

[AutoPR compute/resource-manager] Swagger change for getting all availability sets in a subscription and StandardSSD Azure/azure-sdk-for-python#2829

Merged

Azure deleted a comment from dochung4 Jun 28, 2018

lmazuel reviewed Jun 28, 2018

View reviewed changes

Added nextLink to AvailabilitySetListResult

e6bffd7

AutorestCI mentioned this pull request Jun 28, 2018

[AutoPR compute/resource-manager] Swagger change for getting all availability sets in a subscription and StandardSSD Azure/azure-sdk-for-node#3084

Merged

lmazuel approved these changes Jun 29, 2018

View reviewed changes

lmazuel merged commit 3e46d85 into Azure:master Jun 30, 2018

hyonholee mentioned this pull request Jun 30, 2018

[Compute] Add availability set list by subscription and standard ssd disk Azure/azure-sdk-for-net#4517

Merged

9 tasks

anuchandy mentioned this pull request Jul 2, 2018

User identities in a new dictionary property #3183

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swagger change for getting all availability sets in a subscription and StandardSSD #3302

Swagger change for getting all availability sets in a subscription and StandardSSD #3302

dochung4 commented Jun 25, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

lmazuel Jun 25, 2018

dochung4 Jun 28, 2018

lmazuel Jun 25, 2018

lmazuel Jun 28, 2018

dochung4 Jun 28, 2018

lmazuel Jun 28, 2018

lmazuel Jun 28, 2018 •

edited

Loading

dochung4 Jun 28, 2018

lmazuel Jun 28, 2018 •

edited

Loading

dochung4 Jun 29, 2018

lmazuel commented Jun 29, 2018

dochung4 commented Jun 29, 2018

Swagger change for getting all availability sets in a subscription and StandardSSD #3302

Swagger change for getting all availability sets in a subscription and StandardSSD #3302

Conversation

dochung4 commented Jun 25, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Jun 25, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Jun 25, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Jun 25, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Jun 25, 2018 • edited Loading

Automation for azure-sdk-for-java

AutorestCI commented Jun 25, 2018 • edited Loading

Automation for azure-sdk-for-ruby

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel commented Jun 29, 2018

dochung4 commented Jun 29, 2018

dochung4 commented Jun 25, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

AutorestCI commented Jun 25, 2018 •

edited

Loading

lmazuel Jun 28, 2018 •

edited

Loading

lmazuel Jun 28, 2018 •

edited

Loading