-
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
Creating new API version for ADF #3270
Conversation
2. Remove IR sharing related properties (this feature will not go GA for now) 3. Remove IR API 'removeNode', because IntegrationRuntimeNodes_Delete replaced it.
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/queryPipelineRuns": { | ||
"post": { | ||
"operationId": "PipelineRuns_QueryByFactory", | ||
"x-ms-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.
Should we name this operationId as PipelineRuns_ListByFactory
given this operation is for listing pipe-line -runs List<PipelineRun>
under a factory ? trying to be consistent with general patterns. We even have a linter rule for this.
#WontFix
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.
After discussing with my team leaving as Query #Resolved
}, | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/pipelineruns/{runId}/queryActivityruns": { | ||
"post": { | ||
"operationId": "ActivityRuns_QueryByPipelineRun", |
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.
Should we name this operationId as ActivityRuns_ListByPipelineRun
given this operation is for listing activity-runs List<ActivityRun>
under a pieline run? #WontFix
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.
After discussing with my team leaving as Query #WontFix
{ | ||
"$ref": "#/parameters/runId" | ||
}, | ||
|
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 as previous comment, consider using the operation id TriggerRuns_ListByFactory
to be consistent with the pattern we established in SDKs #WontFix
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.
After discussing with my team leaving as Query #Resolved
{ | ||
"$ref": "#/parameters/runId" | ||
}, | ||
|
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.
using uuid
(guid) is considered as ARM violation - refer this. I've seen other services with identity support uses string instead of uuid. Here is an example. But before making that change, let's see what ARM is recommending here. Since this is a new api-version Gaurav from ARM will be taking a look.
#WontFix
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 change it to string. Just for information, why are GUIDs not recommended? #WontFix
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 just posted about this on slack channel for swagger - since these are for tenantId and principalId properties and not resource names, the format here is fine. In fact it gives an additional type validation on client side, so I will leave these two as GUIDs and add a linter exception. #Resolved
{ | ||
"$ref": "#/parameters/runId" | ||
}, | ||
|
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.
Annotate this with x-ms-enum like:
"x-ms-enum": {
"name": "<name-for-enum-type>",
"modelAsString": false | true
}
Do this for other enums as well. #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.
Which one is this comment referring to? Hard to see. #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've searched through the whole file - the only ones that don't have x-ms-enum are the ones that contain the type name. These translate into a const string which should be intentional.
All others are modeled as enums. #Resolved
Sync-ed with Hermine. There are some changes since last arm review (mainly query part was reviewed last time), hence requesting arm review. #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.
Just one note about upgrade API. Others we had already reviewed and discussed.
} | ||
} | ||
}, | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/upgrade": { |
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.
Upgrade should be modeled as a PUT. @hvermis - I dont recall we discussing this in our conversation. #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.
Removed from swagger for now as per offline discussion. #Resolved
Signing 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