-
Notifications
You must be signed in to change notification settings - Fork 68
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
Update PluginStats with new field in response optional_extended_plugins and upgrade to 2.19.0 #814
Update PluginStats with new field in response optional_extended_plugins and upgrade to 2.19.0 #814
Conversation
Signed-off-by: Craig Perkins <[email protected]>
Changes AnalysisCommit SHA: 68942c6 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/13576974296/artifacts/2666903685 API Coverage
|
Signed-off-by: Craig Perkins <[email protected]>
See tests failing please. Any way we can actually test this field in a response? We have a way to install plugins, you can inspire yourself from https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests/plugins. Update https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml to use 2.19 as stable (replace all 2.18s). Use a 2.20 (even thought there's no plan for such a thing) from dockerhub for the next version. |
Signed-off-by: Craig Perkins <[email protected]>
There is an existing GET I'll see if I can strengthen the existing assertion. |
Is the default test group supposed to contain tests for knn plugin?
|
These tests check all schema. Does the test fail against 2.19 without your change? If so you're all set. |
Yes, the different groups are just different prerequisites. |
Signed-off-by: Craig Perkins <[email protected]>
payload: | ||
nodes: | ||
plugins: | ||
- name: opensearch-cross-cluster-replication |
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.
@dblock I can't find a good way to test this. Is there a way I can assert that the analysis-phonenumber
plugin exists in this list w/o caring about the other entries?
Similarly, how can I make the version
, opensearch_version
and java_version
assertions to be agnostic here?
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.
I don't think the tooling can do this today, I guess don't try too hard. Open an issue for any missing features that could help in the tester.
Should be able to include the type of the expectation, not just the value, e.g. to test that version is present and is a string:
info:
version:
type: string
additionalProperties: true
fun one to code btw :)
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.
@dblock would there be a way to build a generic contains
assertion into the framework? I'm imagining something very generic like: this response contains the strings analysis-phonenumber
AND optional_extended_plugins
in the body of the response.
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 a lot of JSON schema validation here which is the example I gave above. I would not try to implement another style validation because it would be reinventing the wheel, so instead I suggest extending the existing exact check to support a specific sub-schema.
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.
btw, @cwperks if you want to try and implement this, start by adding a test to tools/tests/tester/ResponsePayloadEvaluator.test.ts.
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.
I was just looking into that class to see how the payload evaluator work. Scoping out the effort now and hopefully will push another commit later today where we can reliably assert that the new field exists on the response.
… values Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
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.
I am opposed to the “contains” implementation because it invents a whole new DSL and we already have something better that can assert contains, called json schema validation.
Check whether the response matches exactly (what we do today), or whether it also matches another schema:
declared in the test file.
Feel free to open an issue on needing to support contains and split that from the PR.
This reverts commit e555bc2. Signed-off-by: Craig Perkins <[email protected]>
…f target values" This reverts commit e9c9eb1. Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
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.
Needs a little bit of work to turn green.
Signed-off-by: Craig Perkins <[email protected]>
@nhtruong @Xtansia is there a separate effort for the version upgrade in this repo? I'd offer to lend a hand, but I'm not as familiar with the components that have failing tests. |
@cwperks I can take a look at it |
Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
da71086
to
6f7acf8
Compare
Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Thomas Farr <[email protected]>
8c03125
to
fe82e28
Compare
Signed-off-by: Thomas Farr <[email protected]>
6e627f3
to
490f994
Compare
Signed-off-by: Thomas Farr <[email protected]>
0b7859c
to
50f81b7
Compare
Signed-off-by: Thomas Farr <[email protected]>
Spec Test Coverage Analysis
|
Signed-off-by: Thomas Farr <[email protected]>
Description
A new field was added to the output of the response to
GET _cat/plugins
foroptional_extended_plugins
in 2.19.See opensearch-project/OpenSearch#16909 (comment) and opensearch-project/opensearch-go#669
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.