ESQL: Impersonate an old client in upgrade tests#107497
ESQL: Impersonate an old client in upgrade tests#107497nik9000 wants to merge 3 commits intoelastic:mainfrom
Conversation
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`.
x-pack/plugin/core/template-resources/src/main/resources/monitoring-es-mb.json
Show resolved
Hide resolved
| return result; | ||
| } | ||
|
|
||
| private static ExecutableSection modifyExecutableSection(ExecutableSection e) { |
There was a problem hiding this comment.
Remove version from all esql requests if we're running against before 8.14
| } | ||
|
|
||
| @Override | ||
| protected ClientYamlTestClient initClientYamlTestClient( |
There was a problem hiding this comment.
Send the old client signature if we're before 8.14.
|
run elasticsearch-ci/part-2 |
| esql.query: | ||
| body: | ||
| query: 'FROM test | sort emp_no | eval ip = to_ip(coalesce(ip1.keyword, "255.255.255.255")) | keep emp_no, ip' | ||
| version: 2024.04.01 |
There was a problem hiding this comment.
Hm, maybe the version should be randomized once there's multiple versions?
This is okay, but in #107433 I added the version programmatically, which may be the better approach to deal with this (or not, if we're okay with the yaml tests not applying to snapshot and later versions).
|
|
||
| // Versions on and after 8.14 will get a `version` parameter | ||
| def versionUnsupported = bwcVersion -> { | ||
| return bwcVersion.before(Version.fromString("8.14.0")); |
There was a problem hiding this comment.
nit: could be 8.13.3, so that once 8.13.3 is released, we at least run tests against mixed clusters with 8.13.3 nodes the intended way.
| public RequestObjectBuilder build() throws IOException { | ||
| if (isBuilt == false) { | ||
| if (version != null) { | ||
| builder.field("version", version); |
There was a problem hiding this comment.
praise: that's a much better solution than my previous approach.
| if ("false".equals(System.getProperty("tests.version_parameter_unsupported"))) { | ||
| EsqlVersion version = randomFrom(EsqlSpecTestCase.VERSIONS); | ||
| this.version = randomBoolean() ? version.toString() : version.versionStringWithoutEmoji(); | ||
| } |
There was a problem hiding this comment.
nit: would move that out of the builder and into the actual test logic; could be confusing that the builder relies on global state in the constructor.
| for (Map<String, Object> body : doSection.getApiCallSection().getBodies()) { | ||
| body.remove("version"); | ||
| // TODO if the version isn't the earliest we should skip the test - ES isn't going to support it. | ||
| } |
|
Update: since my PR already touches all YAML tests, I cherry-picked this into my PR here. The difference is that my PR sets the version programmatically rather than pinning it in each individual YAML test. |
|
This was incorporated into another PR. |
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
versionin the body. Old clients will instead default to the first version of ESQL. This should allow us to run these tests even whenversionis required.Tests against newer versions of Elasticsearch will then specify a
version.