-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Azure Cognitive Search 2021-04-30-Preview dataplane API update #21993
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 28 commits
cd811b9
854a534
6b0ae1f
24ef6fe
6e7bf5c
f6b54c4
3df7c4b
48939a9
43d2e8d
491ea24
0e24997
fd37d2d
d7edbf9
4fb0d43
d44cc1f
ec75980
5ae129a
21671ad
a12c6f4
ced497b
652d799
d374f50
df266a4
9e7a97e
eb4a7c0
a028323
6c79d29
f8e32bb
ea6f097
d6b00ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6044,6 +6044,11 @@ | |
| }, | ||
| "SemanticSettings": { | ||
| "properties": { | ||
| "defaultConfiguration": { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jeffrey Richter (@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 commentThe 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. |
||
| "type": "string", | ||
| "description": "The name of the semantic configuration to use if none is specified in the query.", | ||
| "x-nullable": true | ||
| }, | ||
| "configurations": { | ||
| "type": "array", | ||
| "items": { | ||
|
|
||
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 anint32orint64integer 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
LintDiffcheck should also be looked intoUh oh!
There was an error while loading. Please reload this page.
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
LintDiffrelate to missingtype:objectspecifications. Our swagger (with three exceptions) has a history of omitting this, so I'll leave those be to remain consistent. I fixed theint32property.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
LintDiffshould be fixed now, though it seems like the CI is blocked by a merge conflict incustom-words.txt