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
This commit is contained in:
Alpar Torok 2019-06-10 13:39:39 +03:00
parent 44aedcf97a
commit 9def454ea9
5 changed files with 48 additions and 57 deletions

View File

@ -257,11 +257,7 @@ class BuildPlugin implements Plugin<Project> {
} }
} }
if (ext.get('buildDocker')) {
(ext.get('requiresDocker') as List<Task>).add(task) (ext.get('requiresDocker') as List<Task>).add(task)
} else {
task.onlyIf { false }
}
} }
protected static void checkDockerVersionRecent(String dockerVersion) { protected static void checkDockerVersionRecent(String dockerVersion) {

View File

@ -116,11 +116,7 @@ public interface TestClusterConfiguration {
} catch (TestClustersException e) { } catch (TestClustersException e) {
throw e; throw e;
} catch (Exception e) { } catch (Exception e) {
if (lastException == null) { throw e;
lastException = e;
} else {
lastException = e;
}
} }
} }
if (conditionMet == false) { if (conditionMet == false) {
@ -129,7 +125,7 @@ public interface TestClusterConfiguration {
if (lastException == null) { if (lastException == null) {
throw new TestClustersException(message); throw new TestClustersException(message);
} else { } else {
throw new TestClustersException(message, lastException); throw new TestClustersException(message + message, lastException);
} }
} }
logger.info( logger.info(

View File

@ -34,6 +34,7 @@ import org.gradle.api.plugins.ExtraPropertiesExtension;
import org.gradle.api.tasks.TaskContainer; import org.gradle.api.tasks.TaskContainer;
import org.gradle.api.tasks.testing.Test; import org.gradle.api.tasks.testing.Test;
import java.io.File;
import java.util.Collections; import java.util.Collections;
import java.util.function.BiConsumer; import java.util.function.BiConsumer;
@ -56,23 +57,25 @@ public class TestFixturesPlugin implements Plugin<Project> {
disableTaskByType(tasks, ThirdPartyAuditTask.class); disableTaskByType(tasks, ThirdPartyAuditTask.class);
disableTaskByType(tasks, JarHellTask.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 buildFixture = project.getTasks().create("buildFixture");
Task pullFixture = project.getTasks().create("pullFixture"); Task pullFixture = project.getTasks().create("pullFixture");
Task preProcessFixture = project.getTasks().create("preProcessFixture"); Task preProcessFixture = project.getTasks().create("preProcessFixture");
buildFixture.dependsOn(preProcessFixture); buildFixture.dependsOn(preProcessFixture);
pullFixture.dependsOn(preProcessFixture); pullFixture.dependsOn(preProcessFixture);
Task postProcessFixture = project.getTasks().create("postProcessFixture"); 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); preProcessFixture.setEnabled(false);
postProcessFixture.setEnabled(false); postProcessFixture.setEnabled(false);
buildFixture.setEnabled(false); buildFixture.setEnabled(false);
pullFixture.setEnabled(false); pullFixture.setEnabled(false);
return; } else {
}
preProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
postProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
project.apply(spec -> spec.plugin(BasePlugin.class)); project.apply(spec -> spec.plugin(BasePlugin.class));
project.apply(spec -> spec.plugin(DockerComposePlugin.class)); project.apply(spec -> spec.plugin(DockerComposePlugin.class));
ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class); ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class);
@ -87,7 +90,6 @@ public class TestFixturesPlugin implements Plugin<Project> {
pullFixture.dependsOn(tasks.getByName("composePull")); pullFixture.dependsOn(tasks.getByName("composePull"));
tasks.getByName("composeUp").mustRunAfter(preProcessFixture); tasks.getByName("composeUp").mustRunAfter(preProcessFixture);
tasks.getByName("composePull").mustRunAfter(preProcessFixture); tasks.getByName("composePull").mustRunAfter(preProcessFixture);
postProcessFixture.dependsOn(buildFixture);
configureServiceInfoForTask( configureServiceInfoForTask(
postProcessFixture, postProcessFixture,
@ -95,7 +97,7 @@ public class TestFixturesPlugin implements Plugin<Project> {
(name, port) -> postProcessFixture.getExtensions() (name, port) -> postProcessFixture.getExtensions()
.getByType(ExtraPropertiesExtension.class).set(name, port) .getByType(ExtraPropertiesExtension.class).set(name, port)
); );
extension.fixtures.add(project); }
} }
extension.fixtures extension.fixtures
@ -107,7 +109,7 @@ public class TestFixturesPlugin implements Plugin<Project> {
conditionTaskByType(tasks, extension, TestingConventionsTasks.class); conditionTaskByType(tasks, extension, TestingConventionsTasks.class);
conditionTaskByType(tasks, extension, ComposeUp.class); conditionTaskByType(tasks, extension, ComposeUp.class);
if (dockerComposeSupported(project) == false) { if (dockerComposeSupported() == false) {
project.getLogger().warn( project.getLogger().warn(
"Tests for {} require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " + "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() "but none could be found so these will be skipped", project.getPath()
@ -135,7 +137,9 @@ public class TestFixturesPlugin implements Plugin<Project> {
taskClass, taskClass,
task -> task.onlyIf(spec -> task -> task.onlyIf(spec ->
extension.fixtures.stream() extension.fixtures.stream()
.anyMatch(fixtureProject -> fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false) == false .anyMatch(fixtureProject ->
fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false
) == false
) )
); );
} }
@ -168,12 +172,12 @@ public class TestFixturesPlugin implements Plugin<Project> {
); );
} }
public boolean dockerComposeSupported(Project project) { public static boolean dockerComposeSupported() {
if (OS.current().equals(OS.WINDOWS)) { if (OS.current().equals(OS.WINDOWS)) {
return false; return false;
} }
final boolean hasDockerCompose = project.file("/usr/local/bin/docker-compose").exists() || final boolean hasDockerCompose = (new File("/usr/local/bin/docker-compose")).exists() ||
project.file("/usr/bin/docker-compose").exists(); (new File("/usr/bin/docker-compose").exists());
return hasDockerCompose && Boolean.parseBoolean(System.getProperty("tests.fixture.enabled", "true")); return hasDockerCompose && Boolean.parseBoolean(System.getProperty("tests.fixture.enabled", "true"));
} }

View File

@ -2,6 +2,7 @@ import org.elasticsearch.gradle.BuildPlugin
import org.elasticsearch.gradle.LoggedExec import org.elasticsearch.gradle.LoggedExec
import org.elasticsearch.gradle.MavenFilteringHack import org.elasticsearch.gradle.MavenFilteringHack
import org.elasticsearch.gradle.VersionProperties import org.elasticsearch.gradle.VersionProperties
import org.elasticsearch.gradle.testfixtures.TestFixturesPlugin
apply plugin: 'base' apply plugin: 'base'
apply plugin: 'elasticsearch.test.fixtures' apply plugin: 'elasticsearch.test.fixtures'
@ -77,7 +78,10 @@ void addCopyDockerContextTask(final boolean oss) {
} }
preProcessFixture { preProcessFixture {
// don't add the tasks to build the docker images if we have no way of testing them
if (TestFixturesPlugin.dockerComposeSupported()) {
dependsOn assemble dependsOn assemble
}
} }
postProcessFixture.doLast { postProcessFixture.doLast {

View File

@ -169,24 +169,16 @@ if (useFixture) {
File minioAddressFile = new File(project.buildDir, 'generated-resources/s3Fixture.address') 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 thirdPartyTest {
// and pass its name instead.
task writeMinioAddress {
dependsOn tasks.bundlePlugin, tasks.postProcessFixture dependsOn tasks.bundlePlugin, tasks.postProcessFixture
outputs.file(minioAddressFile) outputs.file(minioAddressFile)
doLast { doFirst {
file(minioAddressFile).text = "${ -> minioAddress.call() }" file(minioAddressFile).text = "${ -> minioAddress.call() }"
} }
} // TODO: this could be a nonInputProperties.systemProperty so we don't need a file
thirdPartyTest {
dependsOn writeMinioAddress
inputs.file(minioAddressFile)
systemProperty 'test.s3.endpoint', minioAddressFile.name systemProperty 'test.s3.endpoint', minioAddressFile.name
} }
BuildPlugin.requireDocker(tasks.thirdPartyTest)
task integTestMinio(type: RestIntegTestTask) { task integTestMinio(type: RestIntegTestTask) {
description = "Runs REST tests using the Minio repository." description = "Runs REST tests using the Minio repository."
dependsOn tasks.bundlePlugin, tasks.postProcessFixture dependsOn tasks.bundlePlugin, tasks.postProcessFixture
@ -200,7 +192,6 @@ if (useFixture) {
} }
} }
check.dependsOn(integTestMinio) check.dependsOn(integTestMinio)
BuildPlugin.requireDocker(tasks.integTestMinio)
testClusters.integTestMinio { testClusters.integTestMinio {
keystore 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey keystore 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey