-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Azure Cognitive Search 2021-04-30-Preview dataplane API update #21993
base: main
Are you sure you want to change the base?
Conversation
Add default semantic config to SearchServiceCreateIndex for 2021-04-30-preview API
Hi, @wenyangfu 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] |
Thank you for your contribution wenyangfu! We will review the pull request and get back to you soon. |
Swagger pipeline can not start as the pull request has merge conflicts. |
Swagger Generation Artifacts
|
Swagger pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
NewApiVersionRequired reason: |
Hi @wenyangfu, Your PR has some issues. Please fix the CI sequentially by following the order of
|
{ | ||
"name": "semanticMaxWaitInMilliseconds", | ||
"in": "query", | ||
"type": "integer", |
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 should have a property "format"
indicating whether this is an int32
or int64
integer 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.
The other warnings raised by the LintDiff
check should also be looked into
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.
The other warnings raised by LintDiff
relate to missing type:object
specifications. Our swagger (with three exceptions) has a history of omitting this, so I'll leave those be to remain consistent. I fixed the int32
property.
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 either fix it now or open (and link) to a tracking issue to clean it up. It's technical debt that leads to the same round of questions in future revisions.
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.
All warnings related to LintDiff
should be fixed now, though it seems like the CI is blocked by a merge conflict in custom-words.txt
@@ -6044,6 +6044,11 @@ | |||
}, | |||
"SemanticSettings": { | |||
"properties": { | |||
"defaultConfiguration": { |
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 will need sign off from the API stewardship board as this is considered a breaking change as it's adding a new optional property that didn't exist before.
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.
We still need to get signoff on the exception from the API board. See the DP swagger process doc for details.
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.
Yes, any change to an API contract requires a new api-version. We used to have an exception for somethings but we no longer do as it was causing too much confusion and people were trying to abuse the system.
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.
@JeffreyRichter, this is news to me. ACS has been adding optional properties to existing API versions for quite some time. As additional data point, we did not get any feedback from our customers that this has been causing confusion. Just to extra confirm, the email exception process that we have agreed upon and used in the past is no longer supported. The reason that I'm asking the team will need to figure out how to schedule additional work to add a new api version which long term also carries extra cost maintenance to the code base.
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, any change to an API contract requires a new api-version - this is just the best thing for customers and customer support services (when working with customers). It simplifies our rules and attempted abuse. I'm sorry this adversely affects ACS but we felt that this was the best choice after weighing all options.
Hi, @wenyangfu. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
Next Steps to MergeNext steps that must be taken to merge this PR:
|
@wenyangfu please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Data Plane API - Pull Request
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous Open API document (swagger) if applicable, and the root paths that have been updated.
This PR adds swagger specs for three dataplane API features for Azure Cognitive Search's 2021-04-30-Preview API: Default semantic configuration, Semantic fallback, and Debug API. A preliminary round of reviews was conducted here.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links