ESQL: Make esql version required in REST requests #107433
Conversation
This will make the CI blow up and show us what needs updating.
alex-spies
force-pushed
the
esql-version-in-integration-tests
branch
from
4dc60c1 to
83bb2f6
Compare
This enables using the version from outside the esql project, esp. in integration tests.
This reverts commit 949c652.
In the ESQL quests for upgrading from older versions of Elasticsearch this will pretend to be and old version of the elasticsearch client and not send a `version` in the body. Old clients will instead default to the first version of ESQL. This should allow us to run these tests even when `version` is required. Tests against newer versions of Elasticsearch will then specify a `version`. Co-authored-by: Nik Everett <[email protected]>
| // TODO: make this required | ||
| // "https://github.com/elastic/elasticsearch/issues/104890" | ||
| // validationException = addValidationError(invalidVersion("is required"), validationException); | ||
| validationException = addValidationError(invalidVersion("is required"), validationException); |
There was a problem hiding this comment.
I want to revert this before merging and create a separate PR for it, so that the actual - breaking - API change becomes a single, small PR that can be easily reverted.
There was a problem hiding this comment.
Nevermind, discussed with @nik9000 : we can still revert just this in case something goes wrong.
There was a problem hiding this comment.
We're going to keep this in to save the few hours of build time. If something goes wrong we can back out just this line if we have to.
This reverts commit cb1bb4f.
Co-authored-by: Nik Everett <[email protected]>
| @@ -44,4 +50,40 @@ private void assertRequestBreakerEmpty() throws Exception { | |||
| */ | |||
| EsqlSpecTestCase.assertRequestBreakerEmpty(); | |||
| } | |||
|
|
|||
| public static Iterable<Object[]> updateEsqlQueryDoSections(Iterable<Object[]> parameters, Function<DoSection, ExecutableSection> modify) | |||
There was a problem hiding this comment.
Refactored x-pack/plugin/esql/qa/server/single-node/src/yamlRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlClientYamlAsyncIT.java a little bit, but failed moving this into a place to be used by multiple yaml tests from :/
| if (version != null) { | ||
| builder.field("version", version); | ||
| } |
There was a problem hiding this comment.
Small improvement stolen from @nik9000
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java
Show resolved
Hide resolved
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
alex-spies
changed the title
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryRequestTests.java
Show resolved
Hide resolved
| // TODO: make this required | ||
| // "https://github.com/elastic/elasticsearch/issues/104890" | ||
| // validationException = addValidationError(invalidVersion("is required"), validationException); | ||
| validationException = addValidationError(invalidVersion("is required"), validationException); |
There was a problem hiding this comment.
We're going to keep this in to save the few hours of build time. If something goes wrong we can back out just this line if we have to.
/_queryin integration tests.For mixed cluster tests, impersonates a language client with version 8.13, the only exception in which we do not require the version to be sent (based on #107497 by @nik9000 ).
Fix #107403
Main part of #104890