-
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
create new TA preview API v3.1-preview.2 #10440
create new TA preview API v3.1-preview.2 #10440
Conversation
…ersion to avoid breaking changes in client side code
Add StringIndex param information and PII Redaction updates to Swagger
[Staging] Swagger Validation Report
Only 10 items are listed, please refer to log for more details.
❌ |
Rule | Message |
---|---|
1020 - AddedEnumValue |
The new version is adding enum value(s) 'InvalidRequest, InvalidArgument, InternalServerError, ServiceUnavailable' from the old version. New: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L413:9 Old: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L462:9 |
1020 - AddedEnumValue |
The new version is adding enum value(s) 'InvalidParameterValue, InvalidRequestBodyFormat, EmptyRequest, MissingInputRecords, InvalidDocument, ModelVersionIncorrect, InvalidDocumentBatch, UnsupportedLanguageCode, InvalidCountryHint' from the old version. New: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L484:9 Old: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L533:9 |
1020 - AddedEnumValue |
The new version is adding enum value(s) 'InvalidRequest, InvalidArgument, InternalServerError, ServiceUnavailable' from the old version. New: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L413:9 Old: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L462:9 |
1020 - AddedEnumValue |
The new version is adding enum value(s) 'InvalidParameterValue, InvalidRequestBodyFormat, EmptyRequest, MissingInputRecords, InvalidDocument, ModelVersionIncorrect, InvalidDocumentBatch, UnsupportedLanguageCode, InvalidCountryHint' from the old version. New: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L484:9 Old: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L533:9 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'code' renamed or removed? New: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L398:7 Old: TextAnalytics/preview/v3.1-preview.1/TextAnalytics.json#L461:7 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'code' renamed or removed? New: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L448:7 Old: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L449:7 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'message' renamed or removed? New: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L448:7 Old: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L449:7 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'target' renamed or removed? New: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L448:7 Old: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L449:7 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'innerError' renamed or removed? New: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L448:7 Old: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L449:7 |
1033 - RemovedProperty |
The new version is missing a property found in the old version. Was 'details' renamed or removed? New: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L448:7 Old: TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json#L449:7 |
️️✔️
LintDiff [Detail]
️✔️
Validation passes for LintDiff.
️️✔️
Avocado [Detail]
️✔️
Validation passes for Avocado.
️️✔️
ModelValidation [Detail]
️✔️
Validation passes for ModelValidation.
️️✔️
SemanticValidation [Detail]
️✔️
Validation passes for SemanticValidation.
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-js - Release
|
Azure CLI Extension Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-java - Release
|
azure-sdk-for-go - Release
|
azure-sdk-for-python - Release
|
azure-sdk-for-net - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python-track2 - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Trenton Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Azure Pipelines successfully started running 1 pipeline(s). |
Can one of the admins verify this patch? |
"enum" : [ "textelement_v8", "unicodecodepoint", "utf16codeunit" ], | ||
"default" : "textelement_v8", | ||
"required": false | ||
}, |
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 see my v3.2-preview.1 PR. It includes v3.1-preview.1 updates that should be incorporated here 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.
I have synced this with that PR and this also follows the same conventions now
Azure Pipelines successfully started running 1 pipeline(s). |
@tjprescott @yangyuan Could you guys take a look at this? |
Azure Pipelines successfully started running 1 pipeline(s). |
You will need to reach out to @lmazuel to be able to merge this with breaking changes. |
"$ref": "#/parameters/MultiLanguageInput" | ||
}, | ||
{ | ||
"$ref": "#/parameters/StringIndexType" |
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 string index type also being added to v3.1-preview.1? My understanding was that it was being added v3.1-preview.2 and onward
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 its being added to v3.1-preview.1 also. I was not in the discussions for it. @thediris might know better
@@ -77,10 +77,16 @@ | |||
"$ref": "#/definitions/EntitiesResult" | |||
} | |||
}, | |||
"default": { |
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.
You probably want to keep the default (e.g. what are you returning on a 429 response?)
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.
Sure I will add those
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.
Added
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 consider keeping the "default" error declaration rather than the individual error codes. If you do want individual error codes, they should be marked with x-ms-error-response
to clearly indicate to autorest and friends that it is an error case/expected to raise an exception.
} | ||
] | ||
}, | ||
"x-ms-parameter-location":"client" |
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 you choosing to make it a client parameter instead of a method parameter?
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.
For python multiapi, it'll be better to have this on the method level. We haven't dealt with client parameters that are required for some versions, and don't exist for others
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.
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.
@johanste do you have any input regarding string index type being a client parameter?
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 believe this was based on the assumption that we wouldn't provide a parameter at all for the encoding. But based on the conversation in the arch board review (scenario where I may want to change it when storing something in a database to be consumed by a different language etc.), it is probably better to (still not exposing it as a parameter in the first release) have it as a per-method parameter.
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 changed it to method. @assafi Please let us know if you have any concerns
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.
Yep. Agreed with everything that's been said. Thanks for the feedback everyone.
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
I don't have permissions to merge this because the Breaking Change check is failing. If BCs are approved, you will need to ask @lmazuel to merge this. |
} | ||
] | ||
}, | ||
"x-ms-parameter-location": "client" |
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 the parameter location for this string index type is still client
Azure Pipelines successfully started running 1 pipeline(s). |
...ication/cognitiveservices/data-plane/TextAnalytics/preview/v3.0-preview.1/TextAnalytics.json
Outdated
Show resolved
Hide resolved
...ication/cognitiveservices/data-plane/TextAnalytics/preview/v3.1-preview.2/TextAnalytics.json
Outdated
Show resolved
Hide resolved
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
"x-ms-enum": { | ||
"name": "TokenSentimentValue", | ||
"modelAsString": false | ||
}, |
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 was this added? cc @iscai-msft
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 changes the name of the generated enum class, and ensures that the enum is non-extensible, I'm assuming those were the reasons it was added, @laramume for confirmation
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.
So it is an implementation of the new guidelines defined here: microsoft/api-guidelines#213. Thanks @iscai-msft.
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'm actually not sure myself what this is. @laramume - please remove this if not needed.
Changes in the PR
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Please follow the link to find more details on PR review process.