Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated comments and introduced AvailabilitySetSkuType … #3971

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

robhat1
Copy link
Contributor

@robhat1 robhat1 commented Sep 20, 2018

Updated comments and introduced AvailabilitySetSkuType to help users use predefined constants for specifying the name of Availability Set SKU.

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

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-js

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

Command: ['/usr/local/bin/autorest', '/tmp/tmpxanh4vot/rest/specification/compute/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpxanh4vot/azure-sdk-for-js', '--typescript', '[email protected]/[email protected]']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4286)
   Loading AutoRest extension '@microsoft.azure/autorest.typescript' (2.0.516->2.0.516)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.51->2.3.51)
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,StandardSSD_LRS,UltraSSD_LRS vs. Standard_LRS,Premium_LRS,StandardSSD_LRS
   at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in c:\publish\autorest.modeler\src\ObjectBuilder.cs:line 150
   at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in c:\publish\autorest.modeler\src\SchemaBuilder.cs:line 217
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in c:\publish\autorest.modeler\src\SchemaBuilder.cs:line 48
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in c:\publish\autorest.modeler\src\SchemaBuilder.cs:line 133
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in c:\publish\autorest.modeler\src\SchemaBuilder.cs:line 133
   at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in c:\publish\autorest.modeler\src\SwaggerModeler.cs:line 348
   at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in c:\publish\autorest.modeler\src\SwaggerModeler.cs:line 66
   at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in c:\publish\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: typescript/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
  Error: Plugin imodeler1 reported failure.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@@ -4312,6 +4312,18 @@
},
"description": "The instance view of a resource."
},
"AvailabilitySetSkuType": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this referenced anywhere, is that expected?

Copy link
Contributor Author

@robhat1 robhat1 Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the main idea is to provide a set of constants to the users to specify possible values for skuName while creating Availability Set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we had used the same SKU object at different places and it does not mean the same when used in context of an AvailabilitySet as compared to VMScaleSet or in GetSKUs APIs. Hence this is just to help customers to tell them what are the possible values when creating an Availability Set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these values go in the Sku.Name field? I don't think adding a new type that isn't referenced is the best way to solve this, isn't updating the description sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not. We don't want users to guess the values and type anything random in there. Do you any side-effect of having it? It seems more helpful than just have comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have rephrased that, since this type isn't referenced the code generators throw it away. If you look at some of the generated PRs you'll see that AvailabilitySetSkuType isn't added to the models. So since you can't update the models the best you can do is update the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had checked the generated C# code. It shows up as a static class with 2 string variables there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it doesn't appear for other languages. @fearthecowboy can you comment on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh nevermind I was looking at the wrong SDK PR.

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-node

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

Command: ['/usr/local/bin/autorest', '/tmp/tmp32ew_3wk/rest/specification/compute/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmp32ew_3wk/sdk', '--nodejs', '[email protected]/[email protected]']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.11.3]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4283)
   Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (2.1.99->2.1.99)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
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,StandardSSD_LRS,UltraSSD_LRS vs. Standard_LRS,Premium_LRS,StandardSSD_LRS
   at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\ObjectBuilder.cs:line 147
   at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SchemaBuilder.cs:line 217
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SchemaBuilder.cs:line 48
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SchemaBuilder.cs:line 133
   at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SchemaBuilder.cs:line 133
   at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SwaggerModeler.cs:line 310
   at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\autorest.modeler\src\SwaggerModeler.cs:line 66
   at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in C:\Users\ci\AppData\Local\Temp\PUBLISHu1dw1\44_20171214T003644\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: nodejs/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
  Error: Plugin imodeler1 reported failure.

@AutorestCI
Copy link

AutorestCI commented Sep 20, 2018

Automation for azure-sdk-for-go

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

@jhendrixMSFT jhendrixMSFT merged commit be88f66 into Azure:master Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants