[HDInsight] Refactoring and bug fixes #3256
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… in casing, 2. added missing valid response code
…p), 2. Fixed inappropriate enum constraints, 3. Moved property definitions to appropriate file
Automation for azure-libraries-for-javaNothing to generate for azure-libraries-for-java |
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-nodeThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
hovsepm
reviewed
Jun 18, 2018
| "responses": { | ||
| "200": { | ||
| "description": "Ok response definition." | ||
| }, |
Contributor
There was a problem hiding this comment.
error:
{ code: 'RESPONSE_STATUS_CODE_NOT_IN_EXAMPLE',
id: 'OAV111',
message: 'Following response status codes "200" for operation "Extension_EnableMonitoring" were present in the swagger spec, however they were not present in x-ms-examples. Please provide them.',
innerErrors: undefined }
error: Found errors in validating the response with statusCode "200" for x-ms-example "Enable cluster monitoring" in operation "Extension_EnableMonitoring".:
Contributor
Author
There was a problem hiding this comment.
Thanks. Added to the example.
hovsepm
reviewed
Jun 18, 2018
| "responses": { | ||
| "200": { | ||
| "description": "Ok response definition." | ||
| }, |
Contributor
There was a problem hiding this comment.
error:
{ code: 'RESPONSE_STATUS_CODE_NOT_IN_EXAMPLE',
id: 'OAV111',
message: 'Following response status codes "200" for operation "Extension_DisableMonitoring" were present in the swagger spec, however they were not present in x-ms-examples. Please provide them.',
innerErrors: undefined }
error: Found errors in validating the response with statusCode "200" for x-ms-example "Enable cluster monitoring" in operation "Extension_DisableMonitoring".:
Contributor
Author
There was a problem hiding this comment.
Thanks. Added to the example.
hovsepm
reviewed
Jun 18, 2018
| }, | ||
| { | ||
| "$ref": "#/parameters/ApplicationNameParameter" | ||
| }, |
Contributor
There was a problem hiding this comment.
{
"id": "1016",
"code": "ConstantStatusHasChanged",
"message": "The 'constant' status changed from the old version to the new.",
"jsonref": "#/paths/~1subscriptions~1{subscriptionId}~1resourceGroups~1{resourceGroupName}~1providers~1Microsoft.HDInsight~1clusters~1{clusterName}~1applications~1{applicationName}/put/applicationName",
"json-path": "#/paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.HDInsight/clusters/{clusterName}/applications/{applicationName}/put/applicationName",
"type": "Error"
}
This is a breaking change in SDKs in each language. The previous version had Enum with a single value "hue". The new one have a free form string. Relates to Put and Delete operations.
Contributor
Author
There was a problem hiding this comment.
This should not have been an enum in the first place. The service accepted values other than "hue" before the enum constraint was introduced, and this would break the existing SDK. Additionally, the values for application name are dynamic, so it wasn't appropriate to use enums for them.
Contributor
|
@wawon-msft please take a look to the breaking changes (they are a few). Do you have plans to upgrade API version in the nearest future? Ideally all these breaking changes should be parked till the next API version change. |
Contributor
Author
|
@hovsepm These changes are breaking, but all of them are bug fixes to the spec. We have plans to upgrade the API version, but we would like to get the current spec to a good state before the fork. |
hovsepm
added
the
DoNotMerge
Contributor
|
@wawon-msft please add all bugfixes and changes that you mentioned over IM and ping me directly for the review. |
Automation for azure-sdk-for-javaEncountered a Subprocess error: (azure-sdk-for-java)
Command: ['/usr/local/bin/autorest', '/tmp/tmpwq4fkdu6/rest/specification/hdinsight/resource-manager/readme.md', '--azure-libraries-for-java-folder=/tmp/tmpwq4fkdu6/sdk', '--java', '--multiapi', '[email protected]/[email protected]', '--verbose'] 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/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
Loading AutoRest extension '@microsoft.azure/autorest.java' (2.1.71->2.1.71)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2018-06-preview"} .
FATAL: System.NotImplementedException: Handling return type 'Map<String, String>' for OtherMethod is not implemented
at AutoRest.Java.Azure.Fluent.Model.OtherMethod.get_InnerReturnType() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/OtherMethods/OtherMethod.cs:line 70
at AutoRest.Java.Azure.Fluent.Model.OtherMethod.get_ReturnModel() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/OtherMethods/OtherMethod.cs:line 82
at AutoRest.Java.Azure.Fluent.Model.OtherMethods.<>c.<get_OtherFluentModels>b__12_0(OtherMethod om) in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/OtherMethods/OtherMethods.cs:line 162
at System.Linq.Enumerable.SelectListIterator`2.MoveNext()
at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
at AutoRest.Java.Azure.Fluent.Model.OtherMethods.get_ImportsForInterface() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/OtherMethods/OtherMethods.cs:line 43
at AutoRest.Java.Azure.Fluent.Model.FluentMethodGroup.get_ImportsForInterface() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/FluentMethodGroup/FluentMethodGroup.cs:line 316
at AutoRest.Java.Azure.Fluent.Model.ProxyFluentMethodGroup.get_ImportsForInterface() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/FluentMethodGroup/ProxyFluentMethodGroup.cs:line 135
at AutoRest.Java.Azure.Fluent.Model.FluentMethodGroupInterfaceModel.get_Imports() in /opt/vsts/work/1/s/src/azurefluent/Model/FluentCommon/FluentMethodGroup/FluentMethodGroupInterfaceModel.cs:line 41
at AutoRest.Java.azurefluent.Templates.FluentCommon.FluentMethodGroupInterfaceTemplate.<ExecuteAsync>d__1.MoveNext() in /opt/vsts/work/1/s/src/obj/Razor/azurefluent/Templates/FluentCommon/FluentMethodGroupInterfaceTemplate.cshtml:line 21
--- 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 AutoRest.Core.CodeGenerator.<Write>d__12.MoveNext()
--- 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 AutoRest.Java.Azure.Fluent.CodeGeneratorJvaf.<Generate>d__6.MoveNext() in /opt/vsts/work/1/s/src/azurefluent/CodeGeneratorJvaf.cs:line 63
--- 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 AutoRest.Java.Program.<ProcessInternal>d__3.MoveNext() in /opt/vsts/work/1/s/src/Program.cs:line 114
--- 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: java/generate - FAILED
FATAL: Error: Plugin java reported failure.
Process() cancelled due to exception : Plugin java reported failure.
Failure during batch task - {"tag":"package-2018-06-preview"} -- Error: Plugin java reported failure..
Error: Plugin java reported failure. |
hovsepm
added
the
WaitForARMFeedback
Contributor
|
@ravbhatnagar could you review the new API version 2018-06-01 for HDInsights in this PR? |
hovsepm
reviewed
Jun 21, 2018
| "tags": [ | ||
| "Applications" | ||
| ], | ||
| "description": "Lists all of the applications HDInsight cluster.", |
Contributor
There was a problem hiding this comment.
Lists all of the applications HDInsight cluster. [](start = 32, length = 48)
Could you check this text string? It does not sound right.
hovsepm
reviewed
Jun 21, 2018
| "$ref": "#/parameters/ApiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { |
Contributor
There was a problem hiding this comment.
please consider adding default: response to each endpoint that will describe error response message structures. https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/creating-swagger.md#default-response . You can take a look to some examples in other swaggers e.g. https://github.com/Azure/azure-rest-api-specs/blob/master/specification/monitor/resource-manager/microsoft.insights/stable/2018-04-16/scheduledQueryRule_API.json#L68
Contributor
Author
There was a problem hiding this comment.
This is helpful. I'll look into this.
hovsepm
reviewed
Jun 21, 2018
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/ApplicationGetProperties" |
Contributor
There was a problem hiding this comment.
ApplicationGetProperties [](start = 51, length = 24)
The name for the object is confusing. I think ApplicationProperties (without Get) should be a better option.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the cluster." | ||
| }, | ||
| "ConfigurationNameParameter": { | ||
| "name": "configurationName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The subscription credentials which uniquely identify Microsoft Azure subscription. The subscription ID forms part of the URI for every service call." | ||
| }, | ||
| "ResourceGroupNameParameter": { | ||
| "name": "resourceGroupName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the resource group." | ||
| }, | ||
| "ClusterNameParameter": { | ||
| "name": "clusterName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the cluster." | ||
| }, | ||
| "ExtensionNameParameter": { | ||
| "name": "extensionName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The subscription credentials which uniquely identify Microsoft Azure subscription. The subscription ID forms part of the URI for every service call." | ||
| }, | ||
| "LocationParameter": { | ||
| "name": "location", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The subscription credentials which uniquely identify Microsoft Azure subscription. The subscription ID forms part of the URI for every service call." | ||
| }, | ||
| "ResourceGroupNameParameter": { | ||
| "name": "resourceGroupName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the resource group." | ||
| }, | ||
| "ClusterNameParameter": { | ||
| "name": "clusterName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the cluster." | ||
| }, | ||
| "ScriptNameParameter": { | ||
| "name": "scriptName", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
hovsepm
reviewed
Jun 21, 2018
| "description": "The name of the script." | ||
| }, | ||
| "ScriptExecutionIdParameter": { | ||
| "name": "scriptExecutionId", |
Contributor
There was a problem hiding this comment.
please add "x-ms-parameter-location": "method" property.
Contributor
|
Please fix all following discrepancy errors: |
hovsepm
suggested changes
Jun 21, 2018
Contributor
hovsepm
left a comment
hovsepm
left a comment
There was a problem hiding this comment.
Please address review comments.
…ed properties, renamed operations, fixed examples, added x-ms-parameter-location to certain parameters, and made usages readonly.
wawon-msft
commented
Jun 22, 2018
| @@ -1,403 +1,408 @@ | |||
| { | |||
Contributor
Author
There was a problem hiding this comment.
Not quite sure what happened with the diff. The changes here were:
- Updated version
- Changed Applications_List to Applications_ListByCluster
- Changed description of Applications_ListByCluster
- Changed ApplicationGetProperties to ApplicationProperties
- Changed casing of Errors
- Added x-ms-parameter-location to parameters
In another commit:
- Fixed bug with missing expected response code
Contributor
|
@wawon-msft please fix following linter errors (FYI model names should be PascalCase while property names should be camelCase) |
hovsepm
reviewed
Jun 22, 2018
| "Extensions" | ||
| ], | ||
| "description": "Creates an HDInsight cluster extension.", | ||
| "operationId": "Extensions_Create", |
Contributor
There was a problem hiding this comment.
"operationId": "Extensions_Create", [](start = 16, length = 35)
if the Update of the extension is allowed and it overwrites the existing one - then this operationId should be Extensions_CreateOrUpdate
Contributor
Author
There was a problem hiding this comment.
We don't allow updates on the extension.
hovsepm
reviewed
Jun 22, 2018
| "tags": [ | ||
| "Regions" | ||
| ], | ||
| "operationId": "Location_ListUsages", |
Contributor
There was a problem hiding this comment.
"operationId": "Location_ListUsages [](start = 16, length = 35)
Location should be plural word here.
Contributor
Author
|
@hovsepm: Those linter errors were partially why we introduced the new API version. The property names had underscores in them, which were not camelcase to the linter. I renamed the file to locations.json, which might have raised "new" errors, but these are resolved with the breaking change (by removing the capabilities API). |
hovsepm
approved these changes
Jun 25, 2018
Contributor
|
@wawon-msft looks good to me. Once ARM team will review and sign-off you PR I'll merge it. |
ravbhatnagar
added
ARMSignedOff
Contributor
Author
|
@hovsepm @ravbhatnagar Thanks so much, guys! |
mccleanp
pushed a commit
that referenced
this pull request
Mar 23, 2022
Used inner Error model from Common Types in DeviceUpdate
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are 7 types of changes in this PR:
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
api-versionin the path should match theapi-versionin the spec).Quality of Swagger