-
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
A series of Public swagger update (modification) #6064
Conversation
update version
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-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonA 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-goA 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-javaEncountered a Subprocess error: (azure-sdk-for-java)
Command: ['/usr/local/bin/autorest', '/tmp/tmpxkumramh/rest/specification/datafactory/resource-manager/readme.md', '--perform-load=false', '--swagger-to-sdk', '--output-artifact=configuration.json', '--input-file=foo', '--output-folder=/tmp/tmp5mro5nd8'] AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Failure:
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4373/node_modules/@microsoft.azure/autorest-core
Error: Unable to start AutoRest Core from /root/.autorest/@microsoft.azure_autorest-core@2.0.4373/node_modules/@microsoft.azure/autorest-core
at main (/opt/node_modules/autorest/dist/app.js:232:19)
at <anonymous>
/root/.autorest/@microsoft.azure_autorest-core@2.0.4373/node_modules/@microsoft.azure/autorest-core/dist/app.js:33
autorest_core_1.Shutdown();
^
ReferenceError: autorest_core_1 is not defined
at process.on (/root/.autorest/@microsoft.azure_autorest-core@2.0.4373/node_modules/@microsoft.azure/autorest-core/dist/app.js:33:5)
at emitOne (events.js:121:20)
at process.emit (events.js:211:7)
at process.emit (/node_modules/source-map-support/source-map-support.js:439:21)
fs.js:612
return binding.close(fd);
^
Error: EBADF: bad file descriptor, close
at Object.fs.closeSync (fs.js:612:18)
at StaticVolumeFile.shutdown (/opt/node_modules/autorest/dist/static-loader.js:352:10)
at StaticFilesystem.shutdown (/opt/node_modules/autorest/dist/static-loader.js:406:17)
at process.exit.n [as exit] (/opt/node_modules/autorest/dist/static-loader.js:169:11)
at printErrorAndExit (/node_modules/source-map-support/source-map-support.js:423:11)
at process.emit (/node_modules/source-map-support/source-map-support.js:435:16)
at process._fatalException (bootstrap_node.js:391:26) |
Can one of the admins verify this patch? |
...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Dataset.json
Outdated
Show resolved
Hide resolved
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Outdated
Show resolved
Hide resolved
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Outdated
Show resolved
Hide resolved
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Outdated
Show resolved
Hide resolved
@Yiliu-microsoft Please make sure to fix all checks as well |
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 address comments
...atafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Dataset.json
Show resolved
Hide resolved
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Show resolved
Hide resolved
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Outdated
Show resolved
Hide resolved
REST Spec PR 'Azure/azure-rest-api-specs#6064' REST Spec PR Author 'Yiliu-microsoft' REST Spec PR Last commit
Automation for azure-sdk-for-netA PR has been created for you: |
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 will need an approval from someone from your team in addition to addressing to couple of breaking changes you have introduced in this PR
@@ -197,7 +330,7 @@ | |||
}, | |||
"JsonFormatFilePattern": { | |||
"description": "JSON format file pattern. A property of JsonFormat.", | |||
"type": "string", | |||
"type": "object", |
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.
breaking change in a stable version?
Any reason this cannot be done in the next API version?
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.
Ok, I invited Hermine to explain this change, please wait a moment.
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.
@shahabhijeet the type is changing from string to object, so it's not API breaking change as our service will accept both.
We could mark the SDK-s with breaking tag if you think it's appropriate, however we had another PR like this that wasn't marked.
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.
@hvermis it is a binary breaking change.
I was concerned about a scenario where your customer has written code against your model as
JsonFormat.FilePattern.ToLower().StartWith("array")
Also what I noticed that you have modeled your enum as "modelAsString", but you would like to make a change from string to object?
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.
@shahabhijeet We'll mark the SDK release as breaking.
@@ -313,7 +446,7 @@ | |||
"CompressionLevel": { | |||
"title": "Microsoft.Azure.Management.DataFactories.Models.CompressionLevel", | |||
"description": "All available compression levels.", | |||
"type": "string", | |||
"type": "object", |
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.
breaking change
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.
same here
...tafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/entityTypes/Pipeline.json
Outdated
Show resolved
Hide 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.
I have asked ARM review for this.
What I would like to know is, can you make such changes in stable APIs?
I would like to understand such process, which will help me in future reviews.
@@ -197,7 +330,7 @@ | |||
}, | |||
"JsonFormatFilePattern": { | |||
"description": "JSON format file pattern. A property of JsonFormat.", | |||
"type": "string", | |||
"type": "object", |
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.
@hvermis it is a binary breaking change.
I was concerned about a scenario where your customer has written code against your model as
JsonFormat.FilePattern.ToLower().StartWith("array")
Also what I noticed that you have modeled your enum as "modelAsString", but you would like to make a change from string to object?
@shahabhijeet All these changes are again to support oneOf scenario. The reason we are changing types to object is so our customers can parametrize those properties. Our UI and backend already allow this, only SDK-s are pending. |
@shahabhijeet and @KrisBash Could you please help get this PR in? It contains a lot of features that our customers are waiting for and we need to release our SDKs. |
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.
Discussion about SDK breaking changes aside, there were quite a few properties changed to type: object, but with string enums and modelAsString=true. That seems spurious?
@@ -1148,10 +1148,11 @@ | |||
}, | |||
"StoredProcedureParameterType": { | |||
"description": "Stored procedure parameter type.", | |||
"type": "string", | |||
"type": "object", |
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.
If all allowed enum values are a string, why change to object?
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 is to support Expressions on these properties. OpenApi 2.0 doesn't support oneOf, and we are forced to use object type in order to support parameterization, which is an important feature of ADF for our customers. It allows them to treat their entities as templates and pass different values for properties.
@@ -1206,7 +1207,7 @@ | |||
"properties": { | |||
"writeBehavior": { | |||
"description": "The write behavior for the operation. Default is 'Insert'.", | |||
"type": "string", | |||
"type": "object", |
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.
Like the above, does this need to be an object if allowed values are all strings?
"type": "object", | ||
"description": "Username to access the SAP server where the table is located. Type: string (or Expression with resultType string)." | ||
}, | ||
"password": { |
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.
Friendly reminder: please ensure that the server never returns secrets on get nor logs requests.
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 is already taken care of - we encrypt all secrets, and our service returns only encrypted values.
@KrisBash This is the PR that we need merged. Can you please finish reviewing this one? I will follow up with Yi to close any others, if they are irrelevant. |
FYI, you can add |
@KrisBash @shahabhijeet We need to release this new SDK with Powershell release that is going out on 6/18. |
@@ -205,6 +205,511 @@ | |||
"type": "object" | |||
} | |||
}, | |||
"ConnectorReadSetting": { |
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 are the type names here Setting vs Settings?
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.
Applies to all names
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 change "description": "Connector read settings." to "description": "Connector read setting."
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.
No objection from ARM, assuming changes are legitimate in SDK
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.