-
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
Swagger Changes to Add DnsZonePartner and Collation into Managed Instance #3875
Conversation
Can one of the admins verify this patch? |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-rubyThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeEncountered a Subprocess error: (azure-sdk-for-node)
Command: ['/usr/local/bin/autorest', '/tmp/tmpu78ps76g/rest/specification/sql/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpu78ps76g/sdk', '--nodejs', '--use=@microsoft.azure/autorest.nodejs@2.1.103'] 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/@microsoft.azure_autorest-core@2.0.4283/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4283)
Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (2.1.103->2.1.103)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
Shutting Down
Shutting Down
FATAL: nodejs/generate - FAILED
FATAL: Error: [Exception] AutoRest extension '@microsoft.azure/autorest.nodejs' terminated.
Process() cancelled due to exception : [Exception] AutoRest extension '@microsoft.azure/autorest.nodejs' terminated. |
@@ -44,7 +46,8 @@ | |||
"fullyQualifiedDomainName" : "testinstance.database.windows.net", | |||
"administratorLogin" : "dummylogin", | |||
"subnetId" : "/subscriptions/20D7082A-0FC7-4468-82BD-542694D5042B/resourceGroups/testrg/providers/Microsoft.Network/virtualNetworks/vnet1/subnets/subnet1", | |||
"state" : "Ready" | |||
"state": "Ready", | |||
"dnsZone": "1b4e2caff2530" |
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.
Is this a realistic-looking dns zone?
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 it is. We generate dns zone with the first part of a new Guid.
@@ -22,7 +22,8 @@ | |||
"state": "Ready", | |||
"vCores": 8, | |||
"storageSizeInGB": 1024, | |||
"licenseType": "Full" | |||
"licenseType": "Full", | |||
"collation": "SQL_Latin1_General_CP1_CI_AS" |
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 like dnsZone is missing from all these other examples
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.
Will address this now
"collation": { | ||
"description": "Collation of the managed instance.", | ||
"type": "string", | ||
"x-ms-mutability": [ |
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 thought this was made readonly now
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, the Collation property is made read-only a three weeks ago. Mike, which sync point you've used for generating the swagger changes?
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.
Minor nitpicks, otherwise approved
"readOnly": true | ||
}, | ||
"dnsZone": { | ||
"description": "The Dns Zone taht the managed instance is in.", |
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.
Typo: taht -> that
"licenseType": "Full" | ||
"licenseType": "Full", | ||
"collation": "SQL_Latin1_General_CP1_CI_AS", | ||
"dnsZone": "1b4e2caff2530" |
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.
nit: please fix indentation
"licenseType": "Full" | ||
"licenseType": "Full", | ||
"collation": "SQL_Latin1_General_CP1_CI_AS", | ||
"dnsZone": "1b4e2caff2530" |
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.
nit: fix indentation
If @jaredmoo is good, I am too ;) |
Automation for azure-sdk-for-jsNothing to generate for azure-sdk-for-js |
Thanks! |
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