Skip to content

Commit 4586f29

Browse files
alpar-talpar-t
authored
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
1 parent 31a37fb commit 4586f29

File tree

5 files changed

+48
-55
lines changed

5 files changed

+48
-55
lines changed

‎buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -256,11 +256,7 @@ class BuildPlugin implements Plugin<Project> {
256256
}
257257
}
258258

259-
if (ext.get('buildDocker')) {
260-
(ext.get('requiresDocker') as List<Task>).add(task)
261-
} else {
262-
task.onlyIf { false }
263-
}
259+
(ext.get('requiresDocker') as List<Task>).add(task)
264260
}
265261

266262
protected static void checkDockerVersionRecent(String dockerVersion) {

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,7 @@ default void waitForConditions(
122122
} catch (TestClustersException e) {
123123
throw e;
124124
} catch (Exception e) {
125-
if (lastException == null) {
126-
lastException = e;
127-
} else {
128-
lastException = e;
129-
}
125+
throw e;
130126
}
131127
}
132128
if (conditionMet == false) {
@@ -135,7 +131,7 @@ default void waitForConditions(
135131
if (lastException == null) {
136132
throw new TestClustersException(message);
137133
} else {
138-
throw new TestClustersException(message, lastException);
134+
throw new TestClustersException(message + message, lastException);
139135
}
140136
}
141137
logger.info(

‎buildSrc/src/main/java/org/elasticsearch/gradle/testfixtures/TestFixturesPlugin.java

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.gradle.api.tasks.TaskContainer;
3737
import org.gradle.api.tasks.testing.Test;
3838

39+
import java.io.File;
3940
import java.util.Collections;
4041
import java.util.function.BiConsumer;
4142

@@ -58,46 +59,47 @@ public void apply(Project project) {
5859
disableTaskByType(tasks, ThirdPartyAuditTask.class);
5960
disableTaskByType(tasks, JarHellTask.class);
6061

62+
// the project that defined a test fixture can also use it
63+
extension.fixtures.add(project);
64+
6165
Task buildFixture = project.getTasks().create("buildFixture");
6266
Task pullFixture = project.getTasks().create("pullFixture");
6367
Task preProcessFixture = project.getTasks().create("preProcessFixture");
6468
buildFixture.dependsOn(preProcessFixture);
6569
pullFixture.dependsOn(preProcessFixture);
6670
Task postProcessFixture = project.getTasks().create("postProcessFixture");
71+
postProcessFixture.dependsOn(buildFixture);
72+
preProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
73+
postProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
6774

68-
if (dockerComposeSupported(project) == false) {
75+
if (dockerComposeSupported() == false) {
6976
preProcessFixture.setEnabled(false);
7077
postProcessFixture.setEnabled(false);
7178
buildFixture.setEnabled(false);
7279
pullFixture.setEnabled(false);
73-
return;
74-
}
75-
preProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
76-
postProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
77-
78-
project.apply(spec -> spec.plugin(BasePlugin.class));
79-
project.apply(spec -> spec.plugin(DockerComposePlugin.class));
80-
ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class);
81-
composeExtension.setUseComposeFiles(Collections.singletonList(DOCKER_COMPOSE_YML));
82-
composeExtension.setRemoveContainers(true);
83-
composeExtension.setExecutable(
84-
project.file("/usr/local/bin/docker-compose").exists() ?
85-
"/usr/local/bin/docker-compose" : "/usr/bin/docker-compose"
86-
);
80+
} else {
81+
project.apply(spec -> spec.plugin(BasePlugin.class));
82+
project.apply(spec -> spec.plugin(DockerComposePlugin.class));
83+
ComposeExtension composeExtension = project.getExtensions().getByType(ComposeExtension.class);
84+
composeExtension.setUseComposeFiles(Collections.singletonList(DOCKER_COMPOSE_YML));
85+
composeExtension.setRemoveContainers(true);
86+
composeExtension.setExecutable(
87+
project.file("/usr/local/bin/docker-compose").exists() ?
88+
"/usr/local/bin/docker-compose" : "/usr/bin/docker-compose"
89+
);
8790

88-
buildFixture.dependsOn(tasks.getByName("composeUp"));
89-
pullFixture.dependsOn(tasks.getByName("composePull"));
90-
tasks.getByName("composeUp").mustRunAfter(preProcessFixture);
91-
tasks.getByName("composePull").mustRunAfter(preProcessFixture);
92-
postProcessFixture.dependsOn(buildFixture);
91+
buildFixture.dependsOn(tasks.getByName("composeUp"));
92+
pullFixture.dependsOn(tasks.getByName("composePull"));
93+
tasks.getByName("composeUp").mustRunAfter(preProcessFixture);
94+
tasks.getByName("composePull").mustRunAfter(preProcessFixture);
9395

94-
configureServiceInfoForTask(
95-
postProcessFixture,
96-
project,
97-
(name, port) -> postProcessFixture.getExtensions()
98-
.getByType(ExtraPropertiesExtension.class).set(name, port)
99-
);
100-
extension.fixtures.add(project);
96+
configureServiceInfoForTask(
97+
postProcessFixture,
98+
project,
99+
(name, port) -> postProcessFixture.getExtensions()
100+
.getByType(ExtraPropertiesExtension.class).set(name, port)
101+
);
102+
}
101103
}
102104

103105
extension.fixtures
@@ -109,7 +111,7 @@ public void apply(Project project) {
109111
conditionTaskByType(tasks, extension, TestingConventionsTasks.class);
110112
conditionTaskByType(tasks, extension, ComposeUp.class);
111113

112-
if (dockerComposeSupported(project) == false) {
114+
if (dockerComposeSupported() == false) {
113115
project.getLogger().warn(
114116
"Tests for {} require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " +
115117
"but none could be found so these will be skipped", project.getPath()
@@ -138,7 +140,9 @@ private void conditionTaskByType(TaskContainer tasks, TestFixtureExtension exten
138140
taskClass,
139141
task -> task.onlyIf(spec ->
140142
extension.fixtures.stream()
141-
.anyMatch(fixtureProject -> fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false) == false
143+
.anyMatch(fixtureProject ->
144+
fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false
145+
) == false
142146
)
143147
);
144148
}
@@ -175,12 +179,12 @@ public void execute(Task theTask) {
175179
);
176180
}
177181

178-
public boolean dockerComposeSupported(Project project) {
182+
public static boolean dockerComposeSupported() {
179183
if (OS.current().equals(OS.WINDOWS)) {
180184
return false;
181185
}
182-
final boolean hasDockerCompose = project.file("/usr/local/bin/docker-compose").exists() ||
183-
project.file("/usr/bin/docker-compose").exists();
186+
final boolean hasDockerCompose = (new File("/usr/local/bin/docker-compose")).exists() ||
187+
(new File("/usr/bin/docker-compose").exists());
184188
return hasDockerCompose && Boolean.parseBoolean(System.getProperty("tests.fixture.enabled", "true"));
185189
}
186190

‎distribution/docker/build.gradle

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import org.elasticsearch.gradle.BuildPlugin
22
import org.elasticsearch.gradle.LoggedExec
33
import org.elasticsearch.gradle.MavenFilteringHack
44
import org.elasticsearch.gradle.VersionProperties
5+
import org.elasticsearch.gradle.testfixtures.TestFixturesPlugin
56

67
apply plugin: 'base'
78
apply plugin: 'elasticsearch.test.fixtures'
@@ -77,7 +78,10 @@ void addCopyDockerContextTask(final boolean oss) {
7778
}
7879

7980
preProcessFixture {
80-
dependsOn assemble
81+
// don't add the tasks to build the docker images if we have no way of testing them
82+
if (TestFixturesPlugin.dockerComposeSupported()) {
83+
dependsOn assemble
84+
}
8185
}
8286

8387
postProcessFixture.doLast {

‎plugins/repository-s3/build.gradle

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -169,22 +169,16 @@ if (useFixture) {
169169

170170
File minioAddressFile = new File(project.buildDir, 'generated-resources/s3Fixture.address')
171171

172-
// We can't lazy evaluate a system property for the Minio address passed to JUnit so we write it to a resource file
173-
// and pass its name instead.
174-
task writeMinioAddress {
172+
thirdPartyTest {
175173
dependsOn tasks.bundlePlugin, tasks.postProcessFixture
176-
doLast {
174+
outputs.file(minioAddressFile)
175+
doFirst {
177176
file(minioAddressFile).text = "${ -> minioAddress.call() }"
178177
}
179-
}
180-
181-
thirdPartyTest {
182-
dependsOn writeMinioAddress
178+
// TODO: this could be a nonInputProperties.systemProperty so we don't need a file
183179
systemProperty 'test.s3.endpoint', minioAddressFile.name
184180
}
185181

186-
BuildPlugin.requireDocker(tasks.thirdPartyTest)
187-
188182
task integTestMinio(type: RestIntegTestTask) {
189183
description = "Runs REST tests using the Minio repository."
190184
dependsOn tasks.bundlePlugin, tasks.postProcessFixture
@@ -198,7 +192,6 @@ if (useFixture) {
198192
}
199193
}
200194
check.dependsOn(integTestMinio)
201-
BuildPlugin.requireDocker(tasks.integTestMinio)
202195

203196
testClusters.integTestMinio {
204197
keystore 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey

0 commit comments

Comments
 (0)