-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
2018-05-01 APIs for Microsoft.Resources #2976
Conversation
Automation for azure-sdk-for-nodeEncountered a Subprocess error: (azure-sdk-for-node)
Command: ['/usr/local/bin/autorest', '/tmp/tmpr9dc_x3p/rest/specification/resources/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpr9dc_x3p/sdk', '--nodejs', '[email protected]/autorest.nodejs@^2.1.43'] AutoRest code generation utility [version: 2.0.4262; 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.4278)
Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.59)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
[Exception] No input files provided.
Use --help to get help information. |
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/resources/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/resources/resource-manager/readme.md
|
"tags": [ | ||
"Deployments" | ||
], | ||
"operationId": "Deployments_Delete", |
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.
Deployments_Delete [](start = 24, length = 18)
We should use a different operation Id, so that it doesn't conflict with the existing RG level deployment APIs below
"/subscriptions/{subscriptionId}/providers/Microsoft.Resources/deployments/{deploymentName}": { | ||
"delete": { | ||
"tags": [ | ||
"Deployments" |
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.
Deployments [](start = 11, length = 11)
Deployments_SubscriptionLevel ? (or something better, to differentiate from the original APIs)
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/resources/resource-manager/readme.md
|
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/resources/resource-manager/readme.md
|
"$ref": "#/parameters/SubscriptionIdParameter" | ||
} | ||
], | ||
"responses": { |
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.
Add a "default" error response to capture all the error states. Example here - https://github.com/Azure/azure-rest-api-specs/blob/master/specification/batch/resource-manager/Microsoft.Batch/stable/2017-09-01/BatchManagement.json#L101
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 don't have a good story for the default error responses today. This is common for all our APIs. I think we can address that separately.
@@ -2046,6 +2361,10 @@ | |||
}, | |||
"Deployment": { | |||
"properties": { | |||
"location": { |
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 cant make this change in existing API-version. Existing SDKs will not be able to deserialize.
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.
Right. I'm adding it to a new api-version.
"in": "body", | ||
"required": true, | ||
"schema": { | ||
"$ref": "#/definitions/Deployment" |
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 know this is legacy but why do we have input model different from output. LIke in input we take Deployment and in output we give back DeploymentExtended. Couldnt we have used the same one and if there are properties which are not settable, we could have marked them as readonly.
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.
This was fixed.
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
@anuchandy, @mcardosos this PR is ready for review. Please help take a look. |
|
||
``` yaml $(tag) == 'package-resources-2018-05' | ||
input-file: | ||
- Microsoft.Resources/stable/2018-05-01/resources.json |
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.
@Tiano2017 can you get changes from upstream.
Now CSharp specific config file has been added
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/resources/resource-manager/readme.csharp.md
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.
done. @shahabhijeet can you take one more look?
@amarzavery do you have any idea why nod.js is failing? |
@shahabhijeet are you ok with the C# config changes now? |
@Tiano2017 just talked to @shahabhijeet, he has couple of questions around C# config section, please sync with him. |
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.
Looks good
@anuchandy Synced up with Abhijeet, and he signed off. |
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-version
in the path should match theapi-version
in the spec).Quality of Swagger