Skip to content
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

[Search] Enable OcrSkill and ImageAnalysisSkill #23495

Merged
merged 5 commits into from
Oct 31, 2022

Conversation

dgetu
Copy link
Member

@dgetu dgetu commented Oct 12, 2022

(Purportedly) fixes #23263

@ghost ghost added the Search label Oct 12, 2022
@dgetu dgetu marked this pull request as ready for review October 12, 2022 21:44
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-search-documents

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this PR to be mostly adding some new skill config, but it looks like we're doing rather broad property renames?

@dgetu dgetu force-pushed the search/enable-ocrskill-and-imageanalysisskill branch from 0058207 to 0082b2d Compare October 17, 2022 20:28
@dgetu dgetu requested a review from xirzec October 17, 2022 20:33
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch about the transform issue!

I think this PR is pretty much good to merge, just some housekeeping with mentioning the breaking change of the odatatype

@@ -247,7 +247,7 @@ directive:
```yaml
directive:
- from: swagger-document
where: $.definitions..properties["@odata.etag"]
where: $.definitions..properties["`@odata.etag"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that backtick is the escape character for swagger transforms and that @ is apparently reserved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not specified in the OpenAPI or JSONPath specs, but in the docs for jsonpath-plus, the library which Autorest depends on. Isn't that cool and easy to find?

Copy link
Member

@sarangan12 sarangan12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dgetu dgetu merged commit f7addb6 into Azure:main Oct 31, 2022
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 13, 2023
Update 2023-03-01 to sync with RPSaaSMaster (Azure#23495)

* Adds base for updating Microsoft.AzureStackHCI from version stable/2023-02-01 to version 2023-03-01

* Updates readme

* Updates API version in new specs and examples

* Consent and Disable APIs added

* add managed by for extensions

* prettier fix

* Update cluster resource json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable OcrSkill and ImageAnalysisSkill and move to the latest preview swagger version in the search SDK
4 participants