Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate docker availability build logic #52548

Merged
merged 6 commits into from
Feb 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public String toString() {
private final Property<Platform> platform;
private final Property<Flavor> flavor;
private final Property<Boolean> bundledJdk;
private final Property<Boolean> required;
private final Property<Boolean> failIfUnavailable;

ElasticsearchDistribution(
String name,
Expand All @@ -141,7 +141,7 @@ public String toString() {
this.platform = objectFactory.property(Platform.class);
this.flavor = objectFactory.property(Flavor.class);
this.bundledJdk = objectFactory.property(Boolean.class);
this.required = objectFactory.property(Boolean.class).convention(true);
this.failIfUnavailable = objectFactory.property(Boolean.class).convention(true);
this.extracted = new Extracted(extractedConfiguration);
}

Expand Down Expand Up @@ -190,12 +190,12 @@ public void setBundledJdk(Boolean bundledJdk) {
this.bundledJdk.set(bundledJdk);
}

public boolean isRequired() {
return this.required.get();
public boolean getFailIfUnavailable() {
return this.failIfUnavailable.get();
}

public void setRequired(boolean required) {
this.required.set(required);
public void setFailIfUnavailable(boolean failIfUnavailable) {
this.failIfUnavailable.set(failIfUnavailable);
}

@Override
Expand All @@ -220,7 +220,7 @@ public Extracted getExtracted() {
@Override
public TaskDependency getBuildDependencies() {
// For non-required Docker distributions, skip building the distribution is Docker is unavailable
if (getType() == Type.DOCKER && isRequired() == false && dockerSupport.get().getDockerAvailability().isAvailable == false) {
if (getType() == Type.DOCKER && getFailIfUnavailable() == false && dockerSupport.get().getDockerAvailability().isAvailable == false) {
return task -> Collections.emptySet();
}

Expand Down Expand Up @@ -259,6 +259,12 @@ void finalizeValues() {
return;
}

if (getType() != Type.DOCKER && failIfUnavailable.get() == false) {
throw new IllegalArgumentException(
"failIfUnavailable not allowed for elasticsearch distribution [" + name + "] of type [" + getType() + "]"
);
}

if (getType() == Type.ARCHIVE) {
// defaults for archive, set here instead of via convention so integ-test-zip can verify they are not set
if (platform.isPresent() == false) {
Expand All @@ -270,6 +276,11 @@ void finalizeValues() {
"platform not allowed for elasticsearch distribution [" + name + "] of type [" + getType() + "]"
);
}
if (getType() == Type.DOCKER && bundledJdk.isPresent()) {
throw new IllegalArgumentException(
"bundledJdk not allowed for elasticsearch distribution [" + name + "] of type [docker]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bundledJdk setting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or property or something like that? as it is written it sounds like docker can't use the bundledJdk, when in face it only uses it.

);
}
}

if (flavor.isPresent() == false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
import java.util.stream.Collectors;

/**
* <p>Plugin providing {@link DockerSupportService} for detecting Docker installations and determining requirements for Docker-based
* Elasticsearch build tasks.</p>
*
* <p>Additionally registers a task graph listener used to assert a compatible Docker installation exists when task requiring Docker are
* Plugin providing {@link DockerSupportService} for detecting Docker installations and determining requirements for Docker-based
* Elasticsearch build tasks.
* <p>
* Additionally registers a task graph listener used to assert a compatible Docker installation exists when task requiring Docker are
* scheduled for execution. Tasks may declare a Docker requirement via an extra property. If a compatible Docker installation is not
* available on the build system an exception will be thrown prior to task execution.</p>
* available on the build system an exception will be thrown prior to task execution.
*
* <pre>
* task myDockerTask {
Expand Down Expand Up @@ -49,11 +49,11 @@ public void apply(Project project) {
project.getGradle().getTaskGraph().whenReady(graph -> {
List<String> dockerTasks = graph.getAllTasks().stream().filter(task -> {
ExtraPropertiesExtension ext = task.getExtensions().getExtraProperties();
return ext.has(REQUIRES_DOCKER_ATTRIBUTE) && ext.get(REQUIRES_DOCKER_ATTRIBUTE).equals("true");
return ext.has(REQUIRES_DOCKER_ATTRIBUTE) && (boolean) ext.get(REQUIRES_DOCKER_ATTRIBUTE);
}).map(Task::getPath).collect(Collectors.toList());

if (dockerTasks.isEmpty() == false) {
dockerSupportServiceProvider.get().assertDockerIsAvailable(dockerTasks);
dockerSupportServiceProvider.get().failIfDockerUnavailable(dockerTasks);
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public DockerAvailability getDockerAvailability() {
*
* @throws GradleException if Docker is not available. The exception message gives the reason.
*/
void assertDockerIsAvailable(List<String> tasks) {
void failIfDockerUnavailable(List<String> tasks) {
DockerAvailability availability = getDockerAvailability();

// Docker installation is available and compatible
Expand Down Expand Up @@ -255,7 +255,7 @@ private void throwDockerRequiredException(final String message) {

private void throwDockerRequiredException(final String message, Exception e) {
throw new GradleException(
message + "\nyou can address this by attending to the reported issue, " + "removing the offending tasks from being executed.",
message + "\nyou can address this by attending to the reported issue, or removing the offending tasks from being executed.",
e
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,14 +444,16 @@ private static void addDistro(
if (type == Type.ARCHIVE) {
d.setPlatform(platform);
}
d.setBundledJdk(bundledJdk);
if (type != Type.DOCKER) {
d.setBundledJdk(bundledJdk);
}
d.setVersion(version);
});

// Allow us to gracefully omit building Docker distributions if Docker is not available on the system.
// In such a case as we can't build the Docker images we'll simply skip the corresponding tests.
if (type == Type.DOCKER) {
distro.setRequired(false);
distro.setFailIfUnavailable(false);
}

container.add(distro);
Expand Down
2 changes: 1 addition & 1 deletion distribution/docker/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ elasticsearch_distributions {
flavor = distroFlavor
type = 'docker'
version = VersionProperties.getElasticsearch()
required = false // This ensures we don't attempt to build images if docker is unavailable
failIfUnavailable = false // This ensures we don't attempt to build images if docker is unavailable
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion qa/remote-clusters/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ elasticsearch_distributions {
type = 'docker'
flavor = System.getProperty('tests.distribution', 'default')
version = VersionProperties.getElasticsearch()
required = false // This ensures we skip this testing if Docker is unavailable
failIfUnavailable = false // This ensures we skip this testing if Docker is unavailable
}
}

Expand Down