-
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
[DO NOT MERGE] Add normalizer to latest preview API #11954
Conversation
Swagger Validation Report
|
Swagger Generation Artifacts
|
Hi, @robertklee Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected] |
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Outdated
Show resolved
Hide resolved
@@ -2873,6 +2918,44 @@ | |||
"url": "http://lucene.apache.org/core/4_10_3/analyzers-common/org/apache/lucene/analysis/core/StopAnalyzer.html" | |||
} | |||
}, | |||
"CustomNormalizer": { | |||
"discriminator": "@odata.type", |
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 doesn't appear to be any polymorphism here, so why do you need a discriminator? Does our API even require @odata.type
for normalizer definitions?
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're right, I don't think we need the discriminator right now. I'll remove.
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.
We do need @odata.type
for normalizer definitions. We only support #Microsoft.Azure.Search.CustomNormalizer
right now.
Let's hold off on merging this until the API changes are actually deployed. |
Sounds good
… On Dec 4, 2020, at 4:59 PM, Bruce Johnston ***@***.***> wrote:
Let's hold off on merging this until the API changes are actually deployed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#11954 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHXR3JQGWN27L5PETC7TZPTSTGAVRANCNFSM4UNYO7BQ>.
|
Closing this since we're not ready to merge it yet (the feature hasn't been deployed). We'll re-open the PR once the changes are ready. |
@brjohnstmsft Can you re-open this PR since the changes are checked in and scheduled for next deployment? It seems that I don't have reopening rights. T |
I don't see a re-open button on my end either. I might have to push an update (probably a rebase) to get the re-open button to show up. Edit: rebasing didn't work. |
@ishansrivastava90 @robertklee I don't seem to have re-open rights either, but there's no rush -- This shouldn't be merged until the changes are deployed worldwide. |
@ishansrivastava90 @brjohnstmsft If you need me, feel free to ping me once you are ready by commenting here. I can open a new PR if re-open isn't working at that point. Exciting that it's going to deployment soon! :) |
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
This PR exposes
normalizers
to the 2020-06-30-Preview API version. The docs links still need to be updated after they are written and published.Changelog
Please ensure to add changelog with this PR by answering the following questions.
early 2021 (@ishansrivastava90 and @Yahnoosh to confirm)
early 2021 (@ishansrivastava90 and @Yahnoosh to confirm)
(@ishansrivastava90 and @Yahnoosh to confirm)
Contribution checklist:
handing off to @ishansrivastava90 (was not able to get
autoRest
command to work)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.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
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.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.