Testclsuters: convert plugins qa projects #41496
Conversation
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
| private void addSupplier(String name, Map<String, FileSupplier> collector, String key, FileSupplier valueSupplier) { | ||
| requireNonNull(key, name + " key was null when configuring test cluster `" + this + "`"); | ||
| requireNonNull(valueSupplier, name + " value supplier was null when configuring test cluster `" + this + "`"); | ||
| collector.put(key, valueSupplier); |
There was a problem hiding this comment.
Why all these extra levels of indirection? I think this addSupplier, and the other one added here could just be implemented inside keystore that takes a FileSupplier, and the File variant calls that method instead of this indirection.
| keystoreFiles.values().stream() | ||
| .map(each -> each.get()) | ||
| .filter(each -> each == null || each.exists() == false) | ||
| .forEach(each -> { |
There was a problem hiding this comment.
Sorry but these uses of forEach are getting out of hand. Can you please just use a simple loop? This forEach and the one below can simply be:
for (var entry : keystoreFiles.entrySet()) {
File file = entry.getValue().get();
if (file == null) {
throw new IllegalArgumentException("supplied keystoreFile was null when configuring test cluster `" + this + "`")
}
if (file.exists() == false) {
throw new TestClustersException("supplied keystore file " + each + " does not exist, require for " + this);
}
runElaticsearchBinScript("elasticsearch-keystore", "add-file", entry.getKey(), file.toString())
}
It is the same number of lines, yet does not require thinking about the special way that the first forEach actually works (which gives a different error if null was found instead of a non existent file).
| keystoreSetting 'discovery.ec2.access_key', 'ec2_integration_test_access_key' | ||
| keystoreSetting 'discovery.ec2.secret_key', 'ec2_integration_test_secret_key' | ||
| integTest { | ||
| dependsOn ec2Fixture, project(':plugins:discovery-ec2').bundlePlugin |
There was a problem hiding this comment.
I don't think we should add cross project task dependencies here. While I know cluster formation was already using this pattern, it was internal. By keeping it internal, we can improve the implementation without needing to change 20+ projects to have a different syntax.
There was a problem hiding this comment.
I agree and don't like it either, I was planning to address it in a follow up.
| @@ -30,6 +30,7 @@ integTestCluster { | |||
| retries: 10) | |||
| return tmpFile.exists() | |||
| } | |||
| // TODO: systemProperty('tests.cluster', "${-> cluster.transportPortURI }") when migerating to testclusters | |||
There was a problem hiding this comment.
because it's just a reminder for when we convert it to testclusters, it doesn't use it yet, and we continue to set that property for ClusterFormationTasks
|
run elasticsearch-ci/1 |
|
run elasticsearch-ci/1 |
|
@rjernst ready for annother review. Please merge it if you find it acceptable. |
Add testclusters support for files in keystore and convert qa subprojects within plugins.
Add testclusters support for files in keystore and convert qa subprojects within plugins.
Add testclusters support for files in keystore and convert qa subprojects within plugins.
These slipped trough the cracks initially.
With this PR we now run these tests with test-clusters.