-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Expose master version in REST test context #30623
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
Conversation
This commit exposes the master version to the REST test context. This will be needed in a follow-up where the master version will be used to determine whether or not a certain warning header is expected.
Pinging @elastic/es-core-infra |
Here is the intended usage of this which I will implement in a follow-up PR: diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
index 7c6647d65f0..4aa6b4bd479 100644
--- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
+++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
@@ -20,6 +20,7 @@
package org.elasticsearch.test.rest.yaml.section;
import org.apache.logging.log4j.Logger;
+import org.elasticsearch.Version;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Tuple;
@@ -233,7 +234,7 @@ public class DoSection implements ExecutableSection {
}
fail(formatStatusCodeMessage(response, catchStatusCode));
}
- checkWarningHeaders(response.getWarningHeaders());
+ checkWarningHeaders(response.getWarningHeaders(), executionContext.masterVersion());
} catch(ClientYamlTestResponseException e) {
ClientYamlTestResponse restTestResponse = e.getRestTestResponse();
if (!Strings.hasLength(catchParam)) {
@@ -259,7 +260,7 @@ public class DoSection implements ExecutableSection {
/**
* Check that the response contains only the warning headers that we expect.
*/
- void checkWarningHeaders(final List<String> warningHeaders) {
+ void checkWarningHeaders(final List<String> warningHeaders, final Version masterVersion) {
final List<String> unexpected = new ArrayList<>();
final List<String> unmatched = new ArrayList<>();
final List<String> missing = new ArrayList<>();
@@ -269,8 +270,18 @@ public class DoSection implements ExecutableSection {
for (final String header : warningHeaders) {
final Matcher matcher = WARNING_HEADER_PATTERN.matcher(header);
final boolean matches = matcher.matches();
- if (matches) {
- final String message = matcher.group(1);
+ final String message = matcher.group(1);
+ // noinspection StatementWithEmptyBody
+ if (masterVersion.before(Version.V_7_0_0_alpha1)
+ && message.equals("the default number of shards will change from [5] to [1] in 7.0.0; "
+ + "if you wish to continue using the default of [5] shards, "
+ + "you must manage this on the create index request or with an index template")) {
+ /*
+ * This warning header will come back in the vast majority of our tests that create an index. Rather than rewrite our
+ * tests to assert this warning header, we assume that it is expected.
+ */
+ }
+ else if (matches) {
if (expected.remove(message) == false) {
unexpected.add(header);
} |
} | ||
ClientYamlTestClient clientYamlTestClient = initClientYamlTestClient(restSpec, restClient, hosts, esVersion); | ||
final List<HttpHost> hosts = getClusterHosts(); | ||
Tuple<Version, Version> versionVersionTuple = readVersionsFromCatNodes(adminClient()); |
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.
Note here that we use the admin client instead of the plain client. I think this means that we do not need the try/catch block anymore.
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.
left a question, LGTM though
if (ex.getResponse().getStatusLine().getStatusCode() == 403) { | ||
logger.warn("Fallback to simple info '/' request, _cat/nodes is not authorized"); | ||
esVersion = infoVersion; | ||
logger.info("initializing yaml client, minimum es version: [{}] hosts: {}", esVersion, hosts); |
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.
isn't the catch needed anymore? not too familiar with it though, not sure why it's there
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.
seems like this is not needed as we switched to adminClient for this call
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.
Looks like a good start!
This commit exposes the master version to the REST test context. This will be needed in a follow-up where the master version will be used to determine whether or not a certain warning header is expected.
Relates #30587