-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[HDInsight] Add ListBillingSpecs API to HDInsight's stable and preview version #6360
Conversation
SDK Automation [Logs] (Generated from ddf1a22)Go: Azure/azure-sdk-for-go
Java: Azure/azure-sdk-for-java
Python: Azure/azure-sdk-for-python
JavaScript: Azure/azure-sdk-for-js
|
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-jsA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exlucsion -> exclusion
...ion/hdinsight/resource-manager/Microsoft.HDInsight/preview/2015-03-01-preview/locations.json
Outdated
Show resolved
Hide resolved
...tion/hdinsight/resource-manager/Microsoft.HDInsight/stable/2018-06-01-preview/locations.json
Outdated
Show resolved
Hide resolved
Can one of the admins verify this patch? |
LGTM. |
5b385e8
to
f748fe9
Compare
this is adding new Api and properties in preview and stable versions, need ARM review. |
@AutorestCI regenerate azure-sdk-for-go |
@@ -72,6 +72,45 @@ | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there preview
version inside stable
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there
preview
version insidestable
folder?
@idear1203 could you please help me reply this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergey-shandar This is intentional. It’s a little confusing, but per rule #5 here, whether the swagger spec is in the preview or stable folder “is not a direct analog for whether or not an API Version has the ‘-preview’ suffix or not. SDKs that are generated from 'preview' folder items should indicate to their customers in the most idiomatic way that breaking changes may still be coming.”
Basically the short version is that HDInsight is a GA service that still has an API with a -preview tag. Somebody made the decision several years ago to release stable versions of Hydra/Hyak SDKs for .NET, JavaScript, etc. that consumed this the -preview API. We are now releasing stable swagger-based SDKs so we can at least finally deprecate the Hydra/Hyak SDKs. As a result, we are making a commitment that we will not make breaking changes to this swagger spec version so we can GA some swagger-based SDKs.
@yungezz Is there a ETA for ARM feedback? We have been waiting for several days |
@NelsonDaniel Could we get some ARM feedback? This PR is blocking for one week now |
@idear1203 I am not the ARM reviewer, I am the spec reviewer. As stated on top of the PR: "If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them. |
@@ -123,6 +162,140 @@ | |||
} | |||
}, | |||
"readOnly": true | |||
}, | |||
"BillingResponseListResult": { | |||
"description": "The response for the operation to get regional billingSpecs for a subscription.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type field must be added in every model that does not have it. In this case, type: object must be added. For your reference: https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/creating-swagger.md#understanding-the-importance-of-type-keyword-while-defining-model-types #Resolved
"description": "The managed disk billing sku, P30 or S30.", | ||
"type": "string" | ||
}, | ||
"tier": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the description there is a limited set of values this will accept. Would it be better to have an x-ms-enum? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be other values. Let's keep it as it is now
...ion/hdinsight/resource-manager/Microsoft.HDInsight/preview/2015-03-01-preview/locations.json
Show resolved
Hide resolved
@@ -338,6 +377,140 @@ | |||
} | |||
} | |||
} | |||
}, | |||
"BillingResponseListResult": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
} | ||
} | ||
}, | ||
"VmSizeCompatibilityFilterV2": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
} | ||
} | ||
}, | ||
"BillingResources": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
} | ||
} | ||
}, | ||
"BillingMeters": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
} | ||
} | ||
}, | ||
"VmSizeCompatibilityFilterV2": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
"description": "This class represent a single filter object that defines a multidimensional set. The dimensions of this set are Regions, ClusterFlavors, NodeTypes and ClusterVersions. The constraint should be defined based on the following: FilterMode (Exclude vs Include), VMSizes (the vm sizes in affect of exclusion/inclusion) and the ordering of the Filters. Later filters override previous settings if conflicted.", | ||
"properties": { | ||
"filterMode": { | ||
"description": "The filtering mode. Effectively this can enabling or disabling the VM sizes in a particular set.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something other than string make more sense since you have only two states? #Resolved
} | ||
} | ||
}, | ||
"BillingResources": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: object #Resolved
@NelsonDaniel Thank you for the information! We have notified ARM team for a review. Meanwhile, we are updating code per your comments:) |
Hi @NelsonDaniel , after adding type: object, we got the following model validation error. Could you please help take a look?
|
@AutorestCI regenerate azure-sdk-for-go |
Signing off from ARM side. |
Fix model validation errors
Fix model validation errors
Resolved by correct example file: change "Body" to "body" |
], | ||
"x-ms-enum": { | ||
"name": "Tier", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this because our SDK is GA now and it will be a breaking change. We will fix the issue in the next API version. Created a GitHub issue #6505 to track this.
], | ||
"x-ms-enum": { | ||
"name": "Tier", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this because our SDK is GA now and it will be a breaking change. We will fix the issue in the next API version. Created a GitHub issue #6505 to track this.
], | ||
"x-ms-enum": { | ||
"name": "OSType", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to reuse the exising OSType definition here. If we change "modelAsString" as true for this OSType
definition, we will get the following error:
FATAL: System.InvalidOperationException: Swagger document contains two or more x-ms-enum extensions with the same name 'OSType' and different values: Windows,Linux vs. Windows,Linux
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 150
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 182
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 133
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 182
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 133
at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in /opt/vsts/work/1/s/src/SwaggerModeler.cs:line 348
at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in /opt/vsts/work/1/s/src/SwaggerModeler.cs:line 66
at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in /opt/vsts/work/1/s/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: csharp/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
Error: Plugin imodeler1 reported failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definition should also be corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this because our SDK is GA now and it will be a breaking change. We will fix the issue in the next API version. Created a GitHub issue #6505 to track this.
], | ||
"x-ms-enum": { | ||
"name": "Tier", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
], | ||
"x-ms-enum": { | ||
"name": "OSType", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to reuse the exising Tier definition here. If we change "modelAsString" as true for this Tier
definition, we will get the following error:
FATAL: System.InvalidOperationException: Swagger document contains two or more x-ms-enum extensions with the same name 'Tier' and different values: Standard,Premium vs. Standard,Premium
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 150
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 133
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 182
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 133
at AutoRest.Modeler.ObjectBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/ObjectBuilder.cs:line 182
at AutoRest.Modeler.SchemaBuilder.ParentBuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 217
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 48
at AutoRest.Modeler.SchemaBuilder.BuildServiceType(String serviceTypeName, Boolean required) in /opt/vsts/work/1/s/src/SchemaBuilder.cs:line 133
at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in /opt/vsts/work/1/s/src/SwaggerModeler.cs:line 348
at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in /opt/vsts/work/1/s/src/SwaggerModeler.cs:line 66
at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in /opt/vsts/work/1/s/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: csharp/imodeler1 - FAILED
FATAL: Error: Plugin imodeler1 reported failure.
Process() cancelled due to exception : Plugin imodeler1 reported failure.
Error: Plugin imodeler1 reported failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definition should also be corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this because our SDK is GA now and it will be a breaking change. We will fix the issue in the next API version. Created a GitHub issue #6505 to track this.
], | ||
"x-ms-enum": { | ||
"name": "Tier", | ||
"modelAsString": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "modelAsString" : true
@ravbhatnagar, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future x-ms-enums should "modelAsString": true to avoid breaking changes in the generated types. @idear1203 created an issue here #6505
…w version (Azure#6360) * Add ListBillingSpecs API to stable and preview version * Add enum for tier, osType and filtermode * Update HDI_Locations_ListBillingSpecs.json Fix model validation errors * Update HDI_Locations_ListBillingSpecs.json Fix model validation errors
Add ListBillingSpecs API to HDInsight's two api version(stable/2018-06-01-preview, preview/2015-03-01-preview)
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.