Add API spec for Search Response Processors#440
Add API spec for Search Response Processors#440dbwiddis wants to merge 6 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Changes AnalysisCommit SHA: 59f5728 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10051492286/artifacts/1728520593 API Coverage
|
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
dblock
left a comment
There was a problem hiding this comment.
Looks great!
Update CI to pass.
Maybe we can have coverage of all the other processors (a folder with tests/_core/search/pipeline/response_processors.yaml, etc.)? Eventually they will be used as samples for the documentation.
| - path: /_search/pipeline/sorting_pipeline | ||
| method: DELETE | ||
| status: [200, 404] | ||
| version: '>= 2.16' |
There was a problem hiding this comment.
The CI failed because we lock the build to a specific version on dockerhub here - update it to the latest one that also has the new pipeline added.
There was a problem hiding this comment.
The CI failed because we lock the build to a specific version on dockerhub here - update it to the latest one that also has the new pipeline added.
The PR adding this feature has not been merged yet. What is the usual pattern for writing a test for unmerged code, since submitting this API PR is a prerequisite to merging the PR? 🐔 🥚
There was a problem hiding this comment.
I think you have two choices.
- Wait till the PR is merged and there's a build with it.
- Split this PR in 2, one for existing features and one for the new one, and (1) for the new one.
There was a problem hiding this comment.
I had already assumed option 1 was the expectation.
There was a problem hiding this comment.
Thanks @dbwiddis, looking forward to getting all this closed out!
I'm really trying to timebox how much I'm doing on this. I'll follow up with another PR if necessary but I really just need to get some 2.16.0 features in.
I had that thought but then it just seemed like integration testing and this spec didn't seem the right place to do that. Isn't the goal here just to make sure the REST request works? I wrote detailed samples here and it seems having a link in the API docs to one authoritative source is better than duplicating it? |
The goal here is to make sure that the schema matches between requests and responses. When you write integration tests here the OpenAPI spec is validated, not that the server works. |
The spec should be that source. The problem with the documentation as written is that there's no guarantee that it's correct, so our documentation contains a ton of bugs and typos. With running samples in this repo we will be able to export both the reference documentation (opensearch-project/documentation-website#7700) and working samples in 8 different programming languages that are verified to be correct and work. |
I like this idea (I really like it) and have been trying to figure out the best way on Flow Framework to "integ test" our templates, which are "configuration as code". I think a test framework like you have here is a great solution and I'll be writing an issue summarizing it shortly. In the meantime to keep tracking this ask, feel free to open a new issue to build test/demo setup for Search Pipelines and assign it to me. I may not get to it immediately but it'll be a fun weekend project sometime in the next month. |
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Description
Issues Resolved
Fixes #437
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.