From 4586f2928cc3a0f729692b25c54c746ed730dd52 Mon Sep 17 00:00:00 2001 From: Alpar Torok Date: Mon, 10 Jun 2019 13:39:39 +0300 Subject: [PATCH] Clean up configuration when docker isn't available (#42745) We initially added `requireDocker` for a way for tasks to say that they absolutely must have it, like the build docker image tasks. Projects using the test fixtures plugin are not in this both, as the intent with these is that they will be skipped if docker and docker-compose is not available. Before this change we were lenient, the docker image build would succeed but produce nothing. The implementation was also confusing as it was not immediately obvious this was the case due to all the indirection in the code. The reason we have this leniency is that when we added the docker image build, docker was a fairly new requirement for us, and we didn't have it deployed in CI widely enough nor had CI configured to prefer workers with docker when possible. We are in a much better position now. The other reason was other stack teams running `./gradlew assemble` in their respective CI and the possibility of breaking them if docker is not installed. We have been advocating for building specific distros for some time now and I will also send out an additional notice The PR also removes the use of `requireDocker` from tests that actually use test fixtures and are ok without it, and fixes a bug in test fixtures that would cause incorrect configuration and allow some tasks to run when docker was not available and they shouldn't have. Closes #42680 and #42829 see also #42719 --- .../elasticsearch/gradle/BuildPlugin.groovy | 6 +- .../TestClusterConfiguration.java | 8 +-- .../testfixtures/TestFixturesPlugin.java | 68 ++++++++++--------- distribution/docker/build.gradle | 6 +- plugins/repository-s3/build.gradle | 15 ++-- 5 files changed, 48 insertions(+), 55 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy index 9fd4e0b589ef6..4feda1eba8767 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy @@ -256,11 +256,7 @@ class BuildPlugin implements Plugin { } } - if (ext.get('buildDocker')) { - (ext.get('requiresDocker') as List).add(task) - } else { - task.onlyIf { false } - } + (ext.get('requiresDocker') as List).add(task) } protected static void checkDockerVersionRecent(String dockerVersion) { diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java index 253a58c6b96a7..ddd902093fbe3 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/TestClusterConfiguration.java @@ -122,11 +122,7 @@ default void waitForConditions( } catch (TestClustersException e) { throw e; } catch (Exception e) { - if (lastException == null) { - lastException = e; - } else { - lastException = e; - } + throw e; } } if (conditionMet == false) { @@ -135,7 +131,7 @@ default void waitForConditions( if (lastException == null) { throw new TestClustersException(message); } else { - throw new TestClustersException(message, lastException); + throw new TestClustersException(message + message, lastException); } } logger.info( diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java b/buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java index 0313123655afd..b9eaa2ab979c6 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java @@ -36,6 +36,7 @@ import org.gradle.api.tasks.TaskContainer; import org.gradle.api.tasks.testing.Test; +import java.io.File; import java.util.Collections; import java.util.function.BiConsumer; @@ -58,46 +59,47 @@ public void apply(Project project) { disableTaskByType(tasks, ThirdPartyAuditTask.class); disableTaskByType(tasks, JarHellTask.class); + // the project that defined a test fixture can also use it + extension.fixtures.add(project); + Task buildFixture = project.getTasks().create("buildFixture"); Task pullFixture = project.getTasks().create("pullFixture"); Task preProcessFixture = project.getTasks().create("preProcessFixture"); buildFixture.dependsOn(preProcessFixture); pullFixture.dependsOn(preProcessFixture); Task postProcessFixture = project.getTasks().create("postProcessFixture"); + postProcessFixture.dependsOn(buildFixture); + preProcessFixture.onlyIf(spec -> buildFixture.getEnabled()); + postProcessFixture.onlyIf(spec -> buildFixture.getEnabled()); - if (dockerComposeSupported(project) == false) { + if (dockerComposeSupported() == false) { preProcessFixture.setEnabled(false); postProcessFixture.setEnabled(false); buildFixture.setEnabled(false); pullFixture.setEnabled(false); - return; - } - preProcessFixture.onlyIf(spec -> buildFixture.getEnabled()); - postProcessFixture.onlyIf(spec -> buildFixture.getEnabled()); - - project.apply(spec -> spec.plugin(BasePlugin.class)); - project.apply(spec -> spec.plugin(DockerComposePlugin.class)); - ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class); - composeExtension.setUseComposeFiles(Collections.singletonList(DOCKER_COMPOSE_YML)); - composeExtension.setRemoveContainers(true); - composeExtension.setExecutable( - project.file("/usr/local/bin/docker-compose").exists() ? - "/usr/local/bin/docker-compose" : "/usr/bin/docker-compose" - ); + } else { + project.apply(spec -> spec.plugin(BasePlugin.class)); + project.apply(spec -> spec.plugin(DockerComposePlugin.class)); + ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class); + composeExtension.setUseComposeFiles(Collections.singletonList(DOCKER_COMPOSE_YML)); + composeExtension.setRemoveContainers(true); + composeExtension.setExecutable( + project.file("/usr/local/bin/docker-compose").exists() ? + "/usr/local/bin/docker-compose" : "/usr/bin/docker-compose" + ); - buildFixture.dependsOn(tasks.getByName("composeUp")); - pullFixture.dependsOn(tasks.getByName("composePull")); - tasks.getByName("composeUp").mustRunAfter(preProcessFixture); - tasks.getByName("composePull").mustRunAfter(preProcessFixture); - postProcessFixture.dependsOn(buildFixture); + buildFixture.dependsOn(tasks.getByName("composeUp")); + pullFixture.dependsOn(tasks.getByName("composePull")); + tasks.getByName("composeUp").mustRunAfter(preProcessFixture); + tasks.getByName("composePull").mustRunAfter(preProcessFixture); - configureServiceInfoForTask( - postProcessFixture, - project, - (name, port) -> postProcessFixture.getExtensions() - .getByType(ExtraPropertiesExtension.class).set(name, port) - ); - extension.fixtures.add(project); + configureServiceInfoForTask( + postProcessFixture, + project, + (name, port) -> postProcessFixture.getExtensions() + .getByType(ExtraPropertiesExtension.class).set(name, port) + ); + } } extension.fixtures @@ -109,7 +111,7 @@ public void apply(Project project) { conditionTaskByType(tasks, extension, TestingConventionsTasks.class); conditionTaskByType(tasks, extension, ComposeUp.class); - if (dockerComposeSupported(project) == false) { + if (dockerComposeSupported() == false) { project.getLogger().warn( "Tests for {} require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " + "but none could be found so these will be skipped", project.getPath() @@ -138,7 +140,9 @@ private void conditionTaskByType(TaskContainer tasks, TestFixtureExtension exten taskClass, task -> task.onlyIf(spec -> extension.fixtures.stream() - .anyMatch(fixtureProject -> fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false) == false + .anyMatch(fixtureProject -> + fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false + ) == false ) ); } @@ -175,12 +179,12 @@ public void execute(Task theTask) { ); } - public boolean dockerComposeSupported(Project project) { + public static boolean dockerComposeSupported() { if (OS.current().equals(OS.WINDOWS)) { return false; } - final boolean hasDockerCompose = project.file("/usr/local/bin/docker-compose").exists() || - project.file("/usr/bin/docker-compose").exists(); + final boolean hasDockerCompose = (new File("/usr/local/bin/docker-compose")).exists() || + (new File("/usr/bin/docker-compose").exists()); return hasDockerCompose && Boolean.parseBoolean(System.getProperty("tests.fixture.enabled", "true")); } diff --git a/distribution/docker/build.gradle b/distribution/docker/build.gradle index e99012513753a..46f456c8c0493 100644 --- a/distribution/docker/build.gradle +++ b/distribution/docker/build.gradle @@ -2,6 +2,7 @@ import org.elasticsearch.gradle.BuildPlugin import org.elasticsearch.gradle.LoggedExec import org.elasticsearch.gradle.MavenFilteringHack import org.elasticsearch.gradle.VersionProperties +import org.elasticsearch.gradle.testfixtures.TestFixturesPlugin apply plugin: 'base' apply plugin: 'elasticsearch.test.fixtures' @@ -77,7 +78,10 @@ void addCopyDockerContextTask(final boolean oss) { } preProcessFixture { - dependsOn assemble + // don't add the tasks to build the docker images if we have no way of testing them + if (TestFixturesPlugin.dockerComposeSupported()) { + dependsOn assemble + } } postProcessFixture.doLast { diff --git a/plugins/repository-s3/build.gradle b/plugins/repository-s3/build.gradle index 71e50eacfa755..f97d026b1dfce 100644 --- a/plugins/repository-s3/build.gradle +++ b/plugins/repository-s3/build.gradle @@ -169,22 +169,16 @@ if (useFixture) { File minioAddressFile = new File(project.buildDir, 'generated-resources/s3Fixture.address') - // We can't lazy evaluate a system property for the Minio address passed to JUnit so we write it to a resource file - // and pass its name instead. - task writeMinioAddress { + thirdPartyTest { dependsOn tasks.bundlePlugin, tasks.postProcessFixture - doLast { + outputs.file(minioAddressFile) + doFirst { file(minioAddressFile).text = "${ -> minioAddress.call() }" } - } - - thirdPartyTest { - dependsOn writeMinioAddress + // TODO: this could be a nonInputProperties.systemProperty so we don't need a file systemProperty 'test.s3.endpoint', minioAddressFile.name } - BuildPlugin.requireDocker(tasks.thirdPartyTest) - task integTestMinio(type: RestIntegTestTask) { description = "Runs REST tests using the Minio repository." dependsOn tasks.bundlePlugin, tasks.postProcessFixture @@ -198,7 +192,6 @@ if (useFixture) { } } check.dependsOn(integTestMinio) - BuildPlugin.requireDocker(tasks.integTestMinio) testClusters.integTestMinio { keystore 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey