Simplify adding plugins and modules to testclusters (#61886)

There are currently half a dozen ways to add plugins and modules for
test clusters to use. All of them require the calling project to peek
into the plugin or module they want to use to grab its bundlePlugin
task, and then both depend on that task, as well as extract the archive
path the task will produce. This creates cross project dependencies that
are difficult to detect, and if the dependent plugin/module has not yet
been configured, the build will fail because the task does not yet
exist.

This commit makes the plugin and module methods for testclusters
symmetetric, and simply adding a file provider directly, or a project
path that will produce the plugin/module zip. Internally this new
variant uses normal configuration/dependencies across projects to get
the zip artifact. It also has the added benefit of no longer needing the
caller to add to the test task a dependsOn for bundlePlugin task.

Author: rjernst

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy

@@ -28,7 +28,6 @@ import org.elasticsearch.gradle.info.BuildParams
 import org.elasticsearch.gradle.test.RestIntegTestTask
 import org.elasticsearch.gradle.test.RestTestBasePlugin
 import org.elasticsearch.gradle.testclusters.RunTask
-import org.elasticsearch.gradle.testclusters.TestClustersPlugin
 import org.elasticsearch.gradle.util.Util
 import org.gradle.api.InvalidUserDataException
 import org.gradle.api.Plugin
@@ -42,9 +41,6 @@ import org.gradle.api.tasks.SourceSet
 import org.gradle.api.tasks.TaskProvider
 import org.gradle.api.tasks.bundling.Zip
 
-import java.util.regex.Matcher
-import java.util.regex.Pattern
-
 /**
  * Encapsulates build configuration for an Elasticsearch plugin.
  */
@@ -79,10 +75,9 @@ class PluginBuildPlugin implements Plugin<Project> {
             project.extensions.getByType(PluginPropertiesExtension).extendedPlugins.each { pluginName ->
                 // Auto add dependent modules to the test cluster
                 if (project.findProject(":modules:${pluginName}") != null) {
-                    project.integTest.dependsOn(project.project(":modules:${pluginName}").tasks.bundlePlugin)
-                    project.testClusters.integTest.module(
-                            project.project(":modules:${pluginName}").tasks.bundlePlugin.archiveFile
-                    )
+                    project.testClusters.all {
+                        module(":modules:${pluginName}")
+                    }
                 }
             }
             PluginPropertiesExtension extension1 = project.getExtensions().getByType(PluginPropertiesExtension.class)

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchCluster.java

@@ -26,7 +26,6 @@
 import org.gradle.api.NamedDomainObjectContainer;
 import org.gradle.api.Project;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
 import org.gradle.api.provider.Provider;
@@ -36,7 +35,6 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.security.GeneralSecurityException;
@@ -135,33 +133,23 @@ public void setTestDistribution(TestDistribution distribution) {
     }
 
     @Override
-    public void plugin(URI plugin) {
+    public void plugin(Provider<RegularFile> plugin) {
         nodes.all(each -> each.plugin(plugin));
     }
 
     @Override
-    public void plugin(File plugin) {
-        nodes.all(each -> each.plugin(plugin));
-    }
-
-    @Override
-    public void plugin(Provider<URI> plugin) {
-        nodes.all(each -> each.plugin(plugin));
-    }
-
-    @Override
-    public void plugin(RegularFileProperty plugin) {
-        nodes.all(each -> each.plugin(plugin));
+    public void plugin(String pluginProjectPath) {
+        nodes.all(each -> each.plugin(pluginProjectPath));
     }
 
     @Override
-    public void module(File module) {
+    public void module(Provider<RegularFile> module) {
         nodes.all(each -> each.module(module));
     }
 
     @Override
-    public void module(Provider<RegularFile> module) {
-        nodes.all(each -> each.module(module));
+    public void module(String moduleProjectPath) {
+        nodes.all(each -> each.module(moduleProjectPath));
     }
 
     @Override

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java

@@ -37,12 +37,11 @@
 import org.gradle.api.Named;
 import org.gradle.api.NamedDomainObjectContainer;
 import org.gradle.api.Project;
+import org.gradle.api.artifacts.Configuration;
 import org.gradle.api.file.FileTree;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logger;
 import org.gradle.api.logging.Logging;
-import org.gradle.api.provider.Property;
 import org.gradle.api.provider.Provider;
 import org.gradle.api.tasks.Classpath;
 import org.gradle.api.tasks.Input;
@@ -61,7 +60,6 @@
 import java.io.InputStream;
 import java.io.LineNumberReader;
 import java.io.UncheckedIOException;
-import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -70,6 +68,7 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -126,7 +125,8 @@ public class ElasticsearchNode implements TestClusterConfiguration {
     private final Path workingDir;
 
     private final LinkedHashMap<String, Predicate<TestClusterConfiguration>> waitConditions = new LinkedHashMap<>();
-    private final List<Provider<URI>> plugins = new ArrayList<>();
+    private final Map<String, Configuration> pluginAndModuleConfigurations = new HashMap<>();
+    private final List<Provider<File>> plugins = new ArrayList<>();
     private final List<Provider<File>> modules = new ArrayList<>();
     private final LazyPropertyMap<String, CharSequence> settings = new LazyPropertyMap<>("Settings", this);
     private final LazyPropertyMap<String, CharSequence> keystoreSettings = new LazyPropertyMap<>("Keystore", this);
@@ -262,45 +262,50 @@ private void setDistributionType(ElasticsearchDistribution distribution, TestDis
         }
     }
 
-    @Override
-    public void plugin(RegularFileProperty plugin) {
-        this.plugins.add(plugin.map(p -> p.getAsFile().toURI()));
+    // package protected so only TestClustersAware can access
+    @Internal
+    Collection<Configuration> getPluginAndModuleConfigurations() {
+        return pluginAndModuleConfigurations.values();
     }
 
-    @Override
-    public void plugin(Provider<URI> plugin) {
-        requireNonNull(plugin, "Plugin name can't be null");
-        checkFrozen();
-        if (plugins.contains(plugin)) {
-            throw new TestClustersException("Plugin already configured for installation " + plugin);
-        }
-        this.plugins.add(plugin);
+    // creates a configuration to depend on the given plugin project, then wraps that configuration
+    // to grab the zip as a file provider
+    private Provider<RegularFile> maybeCreatePluginOrModuleDependency(String path) {
+        Configuration configuration = pluginAndModuleConfigurations.computeIfAbsent(
+            path,
+            key -> project.getConfigurations()
+                .detachedConfiguration(project.getDependencies().project(Map.of("path", path, "configuration", "zip")))
+        );
+        Provider<File> fileProvider = configuration.getElements()
+            .map(
+                s -> s.stream()
+                    .findFirst()
+                    .orElseThrow(() -> new IllegalStateException("zip configuration of project " + path + " had no files"))
+                    .getAsFile()
+            );
+        return project.getLayout().file(fileProvider);
     }
 
     @Override
-    public void plugin(URI plugin) {
-        Property<URI> uri = project.getObjects().property(URI.class);
-        uri.set(plugin);
-        this.plugin(uri);
+    public void plugin(Provider<RegularFile> plugin) {
+        checkFrozen();
+        this.plugins.add(plugin.map(RegularFile::getAsFile));
     }
 
     @Override
-    public void plugin(File plugin) {
-        Property<URI> uri = project.getObjects().property(URI.class);
-        uri.set(plugin.toURI());
-        this.plugin(uri);
+    public void plugin(String pluginProjectPath) {
+        plugin(maybeCreatePluginOrModuleDependency(pluginProjectPath));
     }
 
     @Override
-    public void module(File module) {
-        RegularFileProperty file = project.getObjects().fileProperty();
-        file.fileValue(module);
-        this.module(file);
+    public void module(Provider<RegularFile> module) {
+        checkFrozen();
+        this.modules.add(module.map(RegularFile::getAsFile));
     }
 
     @Override
-    public void module(Provider<RegularFile> module) {
-        this.modules.add(module.map(RegularFile::getAsFile));
+    public void module(String moduleProjectPath) {
+        module(maybeCreatePluginOrModuleDependency(moduleProjectPath));
     }
 
     @Override
@@ -449,7 +454,7 @@ public synchronized void start() {
                 logToProcessStdout("installing " + plugins.size() + " plugins in a single transaction");
                 final String[] arguments = Stream.concat(
                     Stream.of("install", "--batch"),
-                    plugins.stream().map(Provider::get).map(URI::toString)
+                    plugins.stream().map(Provider::get).map(p -> p.toURI().toString())
                 ).toArray(String[]::new);
                 runElasticsearchBinScript("elasticsearch-plugin", arguments);
             } else {
@@ -1242,10 +1247,7 @@ private Path getExtractedDistributionDir() {
     }
 
     private List<File> getInstalledFileSet(Action<? super PatternFilterable> filter) {
-        return Stream.concat(
-            plugins.stream().map(Provider::get).filter(uri -> uri.getScheme().equalsIgnoreCase("file")).map(File::new),
-            modules.stream().map(p -> p.get())
-        )
+        return Stream.concat(plugins.stream().map(Provider::get), modules.stream().map(Provider::get))
             .filter(File::exists)
             // TODO: We may be able to simplify this with Gradle 5.6
             // https://docs.gradle.org/nightly/release-notes.html#improved-handling-of-zip-archives-on-classpaths
@@ -1255,15 +1257,6 @@ private List<File> getInstalledFileSet(Action<? super PatternFilterable> filter)
             .collect(Collectors.toList());
     }
 
-    @Input
-    public Set<URI> getRemotePlugins() {
-        Set<URI> file = plugins.stream()
-            .map(Provider::get)
-            .filter(uri -> uri.getScheme().equalsIgnoreCase("file") == false)
-            .collect(Collectors.toSet());
-        return file;
-    }
-
     @Classpath
     public List<File> getInstalledClasspath() {
         return getInstalledFileSet(filter -> filter.include("**/*.jar"));

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java

@@ -21,13 +21,11 @@
 import org.elasticsearch.gradle.FileSupplier;
 import org.elasticsearch.gradle.PropertyNormalization;
 import org.gradle.api.file.RegularFile;
-import org.gradle.api.file.RegularFileProperty;
 import org.gradle.api.logging.Logging;
 import org.gradle.api.provider.Provider;
 import org.slf4j.Logger;
 
 import java.io.File;
-import java.net.URI;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
@@ -45,18 +43,14 @@ public interface TestClusterConfiguration {
 
     void setTestDistribution(TestDistribution distribution);
 
-    void plugin(URI plugin);
+    void plugin(Provider<RegularFile> plugin);
 
-    void plugin(File plugin);
-
-    void plugin(Provider<URI> plugin);
-
-    void plugin(RegularFileProperty plugin);
-
-    void module(File module);
+    void plugin(String pluginProjectPath);
 
     void module(Provider<RegularFile> module);
 
+    void module(String moduleProjectPath);
+
     void keystore(String key, String value);
 
     void keystore(String key, Supplier<CharSequence> valueSupplier);

buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClustersAware.java

@@ -19,9 +19,11 @@
 package org.elasticsearch.gradle.testclusters;
 
 import org.gradle.api.Task;
+import org.gradle.api.artifacts.Configuration;
 import org.gradle.api.tasks.Nested;
 
 import java.util.Collection;
+import java.util.concurrent.Callable;
 
 public interface TestClustersAware extends Task {
 
@@ -34,6 +36,7 @@ default void useCluster(ElasticsearchCluster cluster) {
         }
 
         cluster.getNodes().stream().flatMap(node -> node.getDistributions().stream()).forEach(distro -> dependsOn(distro.getExtracted()));
+        cluster.getNodes().forEach(node -> dependsOn((Callable<Collection<Configuration>>) node::getPluginAndModuleConfigurations));
         getClusters().add(cluster);
     }
 

docs/build.gradle

@@ -94,13 +94,7 @@ project.rootProject.subprojects.findAll { it.parent.path == ':plugins' }.each {
   if (subproj.path.startsWith(':plugins:ingest-attachment') && BuildParams.inFipsJvm) {
     return
   }
-  // FIXME
-  subproj.afterEvaluate { // need to wait until the project has been configured
-    testClusters.integTest {
-      plugin subproj.bundlePlugin.archiveFile
-    }
-    tasks.integTest.dependsOn subproj.bundlePlugin
-  }
+  testClusters.integTest.plugin subproj.path
 }
 
 buildRestTests.docs = fileTree(projectDir) {

modules/ingest-common/build.gradle

@@ -40,7 +40,7 @@ restResources {
 testClusters.all {
   // Needed in order to test ingest pipeline templating:
   // (this is because the integTest node is not using default distribution, but only the minimal number of required modules)
-  module project(':modules:lang-mustache').tasks.bundlePlugin.archiveFile
+  module ':modules:lang-mustache'
 }
 
 thirdPartyAudit.ignoreMissingClasses(

modules/kibana/build.gradle

@@ -28,6 +28,6 @@ dependencies {
 }
 
 testClusters.all {
-  module file(project(':modules:reindex').tasks.bundlePlugin.archiveFile)
+  module ':modules:reindex'
 }
 

modules/lang-painless/build.gradle

@@ -27,7 +27,7 @@ esplugin {
 }
 
 testClusters.all {
-  module project(':modules:mapper-extras').tasks.bundlePlugin.archiveFile
+  module ':modules:mapper-extras'
   systemProperty 'es.scripting.update.ctx_in_params', 'false'
   // TODO: remove this once cname is prepended to transport.publish_address by default in 8.0
   systemProperty 'es.transport.cname_in_publish_address', 'true'

modules/rank-eval/build.gradle

@@ -32,5 +32,5 @@ restResources {
 
 testClusters.all {
   // Modules who's integration is explicitly tested in integration tests
-  module project(':modules:lang-mustache').tasks.bundlePlugin.archiveFile
+  module ':modules:lang-mustache'
 }