Skip to content

Commit 9def454

Browse files
alpar-talpar-t
committed
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 44aedcf commit 9def454

File tree

5 files changed

+47
-56
lines changed

5 files changed

+47
-56
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
@@ -257,11 +257,7 @@ class BuildPlugin implements Plugin<Project> {
257257
}
258258
}
259259

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

267263
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
@@ -116,11 +116,7 @@ default void waitForConditions(
116116
} catch (TestClustersException e) {
117117
throw e;
118118
} catch (Exception e) {
119-
if (lastException == null) {
120-
lastException = e;
121-
} else {
122-
lastException = e;
123-
}
119+
throw e;
124120
}
125121
}
126122
if (conditionMet == false) {
@@ -129,7 +125,7 @@ default void waitForConditions(
129125
if (lastException == null) {
130126
throw new TestClustersException(message);
131127
} else {
132-
throw new TestClustersException(message, lastException);
128+
throw new TestClustersException(message + message, lastException);
133129
}
134130
}
135131
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
@@ -34,6 +34,7 @@
3434
import org.gradle.api.tasks.TaskContainer;
3535
import org.gradle.api.tasks.testing.Test;
3636

37+
import java.io.File;
3738
import java.util.Collections;
3839
import java.util.function.BiConsumer;
3940

@@ -56,46 +57,47 @@ public void apply(Project project) {
5657
disableTaskByType(tasks, ThirdPartyAuditTask.class);
5758
disableTaskByType(tasks, JarHellTask.class);
5859

60+
// the project that defined a test fixture can also use it
61+
extension.fixtures.add(project);
62+
5963
Task buildFixture = project.getTasks().create("buildFixture");
6064
Task pullFixture = project.getTasks().create("pullFixture");
6165
Task preProcessFixture = project.getTasks().create("preProcessFixture");
6266
buildFixture.dependsOn(preProcessFixture);
6367
pullFixture.dependsOn(preProcessFixture);
6468
Task postProcessFixture = project.getTasks().create("postProcessFixture");
69+
postProcessFixture.dependsOn(buildFixture);
70+
preProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
71+
postProcessFixture.onlyIf(spec -> buildFixture.getEnabled());
6572

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

86-
buildFixture.dependsOn(tasks.getByName("composeUp"));
87-
pullFixture.dependsOn(tasks.getByName("composePull"));
88-
tasks.getByName("composeUp").mustRunAfter(preProcessFixture);
89-
tasks.getByName("composePull").mustRunAfter(preProcessFixture);
90-
postProcessFixture.dependsOn(buildFixture);
89+
buildFixture.dependsOn(tasks.getByName("composeUp"));
90+
pullFixture.dependsOn(tasks.getByName("composePull"));
91+
tasks.getByName("composeUp").mustRunAfter(preProcessFixture);
92+
tasks.getByName("composePull").mustRunAfter(preProcessFixture);
9193

92-
configureServiceInfoForTask(
93-
postProcessFixture,
94-
project,
95-
(name, port) -> postProcessFixture.getExtensions()
96-
.getByType(ExtraPropertiesExtension.class).set(name, port)
97-
);
98-
extension.fixtures.add(project);
94+
configureServiceInfoForTask(
95+
postProcessFixture,
96+
project,
97+
(name, port) -> postProcessFixture.getExtensions()
98+
.getByType(ExtraPropertiesExtension.class).set(name, port)
99+
);
100+
}
99101
}
100102

101103
extension.fixtures
@@ -107,7 +109,7 @@ public void apply(Project project) {
107109
conditionTaskByType(tasks, extension, TestingConventionsTasks.class);
108110
conditionTaskByType(tasks, extension, ComposeUp.class);
109111

110-
if (dockerComposeSupported(project) == false) {
112+
if (dockerComposeSupported() == false) {
111113
project.getLogger().warn(
112114
"Tests for {} require docker-compose at /usr/local/bin/docker-compose or /usr/bin/docker-compose " +
113115
"but none could be found so these will be skipped", project.getPath()
@@ -135,7 +137,9 @@ private void conditionTaskByType(TaskContainer tasks, TestFixtureExtension exten
135137
taskClass,
136138
task -> task.onlyIf(spec ->
137139
extension.fixtures.stream()
138-
.anyMatch(fixtureProject -> fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false) == false
140+
.anyMatch(fixtureProject ->
141+
fixtureProject.getTasks().getByName("buildFixture").getEnabled() == false
142+
) == false
139143
)
140144
);
141145
}
@@ -168,12 +172,12 @@ private void configureServiceInfoForTask(Task task, Project fixtureProject, BiCo
168172
);
169173
}
170174

171-
public boolean dockerComposeSupported(Project project) {
175+
public static boolean dockerComposeSupported() {
172176
if (OS.current().equals(OS.WINDOWS)) {
173177
return false;
174178
}
175-
final boolean hasDockerCompose = project.file("/usr/local/bin/docker-compose").exists() ||
176-
project.file("/usr/bin/docker-compose").exists();
179+
final boolean hasDockerCompose = (new File("/usr/local/bin/docker-compose")).exists() ||
180+
(new File("/usr/bin/docker-compose").exists());
177181
return hasDockerCompose && Boolean.parseBoolean(System.getProperty("tests.fixture.enabled", "true"));
178182
}
179183

‎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: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -169,24 +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
176174
outputs.file(minioAddressFile)
177-
doLast {
175+
doFirst {
178176
file(minioAddressFile).text = "${ -> minioAddress.call() }"
179177
}
180-
}
181-
182-
thirdPartyTest {
183-
dependsOn writeMinioAddress
184-
inputs.file(minioAddressFile)
178+
// TODO: this could be a nonInputProperties.systemProperty so we don't need a file
185179
systemProperty 'test.s3.endpoint', minioAddressFile.name
186180
}
187181

188-
BuildPlugin.requireDocker(tasks.thirdPartyTest)
189-
190182
task integTestMinio(type: RestIntegTestTask) {
191183
description = "Runs REST tests using the Minio repository."
192184
dependsOn tasks.bundlePlugin, tasks.postProcessFixture
@@ -200,7 +192,6 @@ if (useFixture) {
200192
}
201193
}
202194
check.dependsOn(integTestMinio)
203-
BuildPlugin.requireDocker(tasks.integTestMinio)
204195

205196
testClusters.integTestMinio {
206197
keystore 's3.client.integration_test_permanent.access_key', s3PermanentAccessKey

0 commit comments

Comments
 (0)