-
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
[Data Factory]Add additionalColumns/dataConsistency/sessionLog/isolationLevel/expiryDateTime/fileListPath in ADF public swagger #7947
Conversation
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-js - Release
|
azure-sdk-for-go - Release
|
azure-sdk-for-python - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-net - Release
|
LGTM but please wait for response from ARM review team |
Can one of the admins verify this patch? |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -828,6 +828,13 @@ | |||
"storeSettings": { | |||
"$ref": "#/definitions/StoreReadSettings", | |||
"description": "Avro store settings." | |||
}, | |||
"additionalColumns": { |
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.
Adding new properties to the response of an API is classified as a breaking change. Please make these changes in a new 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.
@ryansbenson We had a discussion with ARM team in August that this kind of change in ADF swagger is not considered as a breaking change. Below is quote from the email between us:
Regarding to the “definition” of breaking change, I think ADF has a looser definition than that of ARM, and looks like it’s allowable from definition of the spec : RestAPI_BreakingChangeGuide.docx
“Teams MAY define backwards compatibility as their business needs require. For example, Azure defines the addition of a new JSON field in a response to be not backwards compatible. Office 365 has a looser definition of backwards compatibility and allows JSON fields to be added to responses.”
Back to ADF business needs, it is an data integration service which support 80+ data stores and certain number of other compute system as well, we model these data stores as LinkedService (with a “type” property to identify the actual data store type), and the each piece of data in these data stores as DataSet (with a “type” property to identify the actual data type), while we have another model object called Pipeline which refer to LinkedServices and DataSets to define activities to process these data. As you can see, in this model design, whenever we add a new type of LinkedService and DataSet, it will result in adding new properties of the API, but these new properties won’t impact the main concept of our model. Moreover, if we always upgrade API version in this case, it will be very difficult for customer to figure out which API version support what data stores given we have large number of data stores and we add this data store support very quickly. So per these business requirements, we decided not to add major version API for new connectors.
Regarding to the swagger review process, I would expect both ARM and Swagger review team can understand this, and we don’t need to explain these details for each single swagger review.
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.
Can you forward me the email thread regarding this? [email protected]
The idea that RP teams can define backwards compatibility it supposed to enable teams to be more restrictive than ARM on changes, not less.
You'll notice the O365 reference is omitted in the ARM breaking changes doc and was replaced by Azures high level view of a breaking change:
"Anything that would violate the Principle of Least Astonishment is considered a breaking change in Azure. Below are some concrete examples of what constitutes a breaking change. In the below breaking change scenarios, the API version must be changed."
The O365 reference comes from the following doc which describes the all up Microsoft breaking change definition:
https://github.com/Microsoft/api-guidelines/blob/vNext/Guidelines.md#123-definition-of-a-breaking-change
When reading the Microsoft docs, Azure is the business and ADF is a component of Azure.
This isn't just a random requirement we added. If you introduce new LinkedService kinds in the same api version, the existing SDK could start failing. ie If one of your new LinkedServices is returned, it will fail to deserialize. If you introduce it in a new API version, the old SDK wouldn't see the new LinkedServices and would continue to work.
If you have an exception for this, you will have to explain this to every reviewer as this is technically a breaking change according to Azure.
Hi @rickysun93 do we have any updates on this PR? |
We're now communicating with ARM team to figure out a final decision on the api version issue, which may cost some time. |
Thanks for the update |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
typically this change requires a new apiversion. But since ADF has been granted an exception in the past, signing off on this. We are still working on closing this with key stakeholders. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
…ionLevel/expiryDateTime/fileListPath in ADF public swagger (Azure#7947) * add properties for ADF public swagger * remove from datasetV1 * remove office365 * add Mds Linkedservice/Dataset/Source and properties in ADLS sink/binary sources
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.