Skip to content

Commit

Permalink
DockerBuilderPublisher now throws a better cloud-not-found error. (#796)
Browse files Browse the repository at this point in the history
Refactor: Ditch JenkinsUtils.getServers() in favour of DockerCloud.instances()
Refactor: Renamed JenkinsUtils.getServer(String) to getCloudByNameOrThrow(String) to better describe what it does.
Refactor: Added getServerNames() to JenkinsUtils and removed implementation of same from DockerManagement.
Enhancement: JenkinsUtils.getCloudByNameOrThrow(String) now throws a human-friendly exception when it doesn't find a cloud of the given name instead of a nasty exception.
...and added unit tests for JenkinsUtils.getCloudByNameOrThrow(String).
  • Loading branch information
pjdarton authored Aug 10, 2020
1 parent 2c45033 commit cd41bf9
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@
import org.slf4j.LoggerFactory;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand Down Expand Up @@ -813,18 +815,19 @@ long getEffectiveErrorDurationInMilliseconds() {
return TimeUnit.SECONDS.toMillis(ERROR_DURATION_DEFAULT_SECONDS);
}

@Nonnull
public static List<DockerCloud> instances() {
List<DockerCloud> instances = new ArrayList<>();
for (Cloud cloud : Jenkins.getInstance().clouds) {
if (cloud instanceof DockerCloud) {
instances.add((DockerCloud) cloud);
}

}
return instances;
}

@Restricted(NoExternalUse.class)
@CheckForNull
static DockerCloud findCloudForTemplate(final DockerTemplate template) {
for (DockerCloud cloud : instances()) {
if ( cloud.hasTemplate(template) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,7 @@ public Object getTarget() {
}

public Collection<String> getServerNames() {
return Collections2.transform(JenkinsUtils.getServers(), new Function<DockerCloud, String>() {
@Override
public String apply(DockerCloud input) {
return input.getDisplayName();
}
});
return JenkinsUtils.getServerNames();
}

public static class ServerDetail {
Expand All @@ -109,13 +104,13 @@ public String getActiveHosts() {
}
return "(" + containers.size() + ")";
} catch (Exception ex) {
return "Error";
return "Error: " + ex;
}
}
}

public Collection<ServerDetail> getServers() {
return Collections2.transform(JenkinsUtils.getServers(), new Function<DockerCloud, ServerDetail>() {
return Collections2.transform(DockerCloud.instances(), new Function<DockerCloud, ServerDetail>() {
@Override
public ServerDetail apply(@Nullable DockerCloud input) {
return new ServerDetail(input);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public String getUrl() {

public DockerManagementServer(String name) {
this.name = name;
theCloud = JenkinsUtils.getServer(name);
theCloud = JenkinsUtils.getCloudByNameOrThrow(name);
}

public Collection getImages(){
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ protected DockerAPI getDockerAPI(Launcher launcher) {
DockerCloud theCloud;
final VirtualChannel channel = launcher.getChannel();
if (!Strings.isNullOrEmpty(cloud)) {
theCloud = JenkinsUtils.getServer(cloud);
theCloud = JenkinsUtils.getCloudByNameOrThrow(cloud);
} else {
if(channel instanceof Channel) {
final Node node = Jenkins.getInstance().getNode(((Channel)channel).getName() );
Expand All @@ -263,7 +263,7 @@ protected DockerAPI getDockerAPI(Launcher launcher) {
// Triton can't do docker build. Ensure we're not trying to do that.
if (theCloud.isTriton()) {
LOGGER.warn("Selected cloud for build does not support this feature. Finding an alternative");
for (DockerCloud dc : JenkinsUtils.getServers()) {
for (DockerCloud dc : DockerCloud.instances()) {
if (!dc.isTriton()) {
LOGGER.warn("Picked {} cloud instead", dc.getDisplayName());
theCloud = dc;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package com.nirima.jenkins.plugins.docker.utils;

import com.google.common.base.Predicate;
import com.google.common.base.Function;
import com.google.common.base.Splitter;
import com.google.common.collect.Collections2;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.nirima.jenkins.plugins.docker.DockerCloud;
import hudson.Launcher;
import hudson.Util;
Expand All @@ -12,7 +11,6 @@
import hudson.model.Run;
import hudson.remoting.Channel;
import hudson.remoting.VirtualChannel;
import hudson.slaves.Cloud;
import io.jenkins.docker.DockerTransientNode;
import jenkins.model.Jenkins;

Expand Down Expand Up @@ -85,27 +83,37 @@ public static Optional<DockerCloud> getCloudThatWeBuiltOn(Run<?, ?> build, Launc
}

/**
* Get the list of Docker servers.
*
* @return the list as a LinkedList of DockerCloud
* Finds the {@link DockerCloud} with the {@link DockerCloud#getDisplayName()}
* matching the specified name.
*
* @param serverName The name to look for.
* @return {@link DockerCloud} with the {@link DockerCloud#getDisplayName()}
* matching the specified name.
* @throws IllegalArgumentException if no {@link DockerCloud} exists with that
* name.
*/
@Restricted(NoExternalUse.class)
public static synchronized Collection<DockerCloud> getServers() {
Collection clouds = Collections2.filter(Jenkins.getInstance().clouds, new Predicate<Cloud>() {
@Override
public boolean apply(Cloud input) {
return input instanceof DockerCloud;
@Nonnull
public static DockerCloud getCloudByNameOrThrow(final String serverName) {
try {
final DockerCloud resultOrNull = DockerCloud.getCloudByName(serverName);
if (resultOrNull != null) {
return resultOrNull;
}
});
return clouds;
} catch (ClassCastException treatedAsCloudNotFound) {
// we found a cloud of that name, but it wasn't one of ours.
}
throw new IllegalArgumentException("No " + DockerCloud.class.getSimpleName() + " with name '" + serverName
+ "'. Known names are " + getServerNames());
}

@Restricted(NoExternalUse.class)
public static DockerCloud getServer(final String serverName) {
return Iterables.find(getServers(), new Predicate<DockerCloud>() {
@Nonnull
public static List<String> getServerNames() {
return Lists.transform(DockerCloud.instances(), new Function<DockerCloud, String>() {
@Override
public boolean apply(DockerCloud input) {
return input != null && serverName.equals(input.getDisplayName());
public String apply(DockerCloud input) {
return input == null ? "" : input.getDisplayName();
}
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package com.nirima.jenkins.plugins.docker.utils;

import static org.junit.Assert.assertThat;
import static org.hamcrest.Matchers.*;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

import org.jenkinsci.plugins.docker.commons.credentials.DockerServerEndpoint;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;

import com.nirima.jenkins.plugins.docker.DockerCloud;

import hudson.model.Label;
import hudson.slaves.Cloud;
import hudson.slaves.NodeProvisioner.PlannedNode;
import io.jenkins.docker.client.DockerAPI;

public class JenkinsUtilsTest {

@Rule
public JenkinsRule jenkins = new JenkinsRule();

@Test
public void getCloudByNameOrThrowGivenNameThenReturnsCloud() throws Exception {
// Given
final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId"));
final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList());
final String expectedCloudName = "expectedCloudName";
final DockerCloud cloudExpected = new DockerCloud(expectedCloudName, dockerApi, Collections.emptyList());
final DockerCloud cloudOther = new DockerCloud("otherCloudName", dockerApi, Collections.emptyList());
final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud("foreign");
final List<Cloud> clouds = Arrays.asList(cloudEmpty, cloudOther, cloudExpected, cloudForeign);
jenkins.getInstance().clouds.replaceBy(clouds);

// When
final DockerCloud actual = JenkinsUtils.getCloudByNameOrThrow(expectedCloudName);

// Then
assertThat(actual, sameInstance(cloudExpected));
}

@Test
public void getCloudByNameOrThrowGivenUnknownNameThenThrows() throws Exception {
// Given
final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId"));
final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList());
final String requestedCloudName = "anUnknownCloudName";
final String cloudName1 = "expectedCloudName";
final DockerCloud cloud1 = new DockerCloud(cloudName1, dockerApi, Collections.emptyList());
final String cloudName2 = "otherCloudName";
final DockerCloud cloud2 = new DockerCloud(cloudName2, dockerApi, Collections.emptyList());
final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud("foreign");
final List<Cloud> clouds = Arrays.asList(cloudEmpty, cloud2, cloud1, cloudForeign);
jenkins.getInstance().clouds.replaceBy(clouds);

try {
// When
JenkinsUtils.getCloudByNameOrThrow(requestedCloudName);
Assert.fail("Expected " + IllegalArgumentException.class.getCanonicalName());
} catch (IllegalArgumentException ex) {
// Then
final String actualMsg = ex.getMessage();
assertThat(actualMsg, containsString(requestedCloudName));
assertThat(actualMsg, containsString(cloudName1));
assertThat(actualMsg, containsString(cloudName2));
assertThat(actualMsg, not(containsString(cloudForeign.name)));
}
}

@Test
public void getCloudByNameOrThrowGivenForeignCloudNameThenThrows() throws Exception {
// Given
final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId"));
final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList());
final String requestedCloudName = "foreign";
final String cloudName1 = "DockerCloud1Name";
final DockerCloud cloud1 = new DockerCloud(cloudName1, dockerApi, Collections.emptyList());
final String cloudName2 = "DockerCloud2Name";
final DockerCloud cloud2 = new DockerCloud(cloudName2, dockerApi, Collections.emptyList());
final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud(requestedCloudName);
final List<Cloud> clouds = Arrays.asList(cloudEmpty, cloud2, cloud1, cloudForeign);
jenkins.getInstance().clouds.replaceBy(clouds);

try {
// When
JenkinsUtils.getCloudByNameOrThrow(requestedCloudName);
Assert.fail("Expected " + IllegalArgumentException.class.getCanonicalName());
} catch (IllegalArgumentException ex) {
// Then
final String actualMsg = ex.getMessage();
assertThat(actualMsg, containsString(requestedCloudName));
assertThat(actualMsg, containsString(cloudName1));
assertThat(actualMsg, containsString(cloudName2));
}
}

private static class OtherTypeOfCloud extends Cloud {
protected OtherTypeOfCloud(String name) {
super(name);
}

@Override
public Collection<PlannedNode> provision(Label label, int excessWorkload) {
return null;
}

@Override
public boolean canProvision(Label label) {
return false;
}
}
}

0 comments on commit cd41bf9

Please sign in to comment.