JENKINS-29136 fix NPE in getRetentionTime()

I documented the bug here: https://issues.jenkins-ci.org/browse/JENKINS-29136 this NPE was preventing jenkins workers from being deleted when they should have been. I added a defaults class that will be used when parent cloud cannot be resolved due to whatever reason.
jenkinsci · Jun 30, 2015 · e4f8cdc · e4f8cdc
1 parent 20c6ca8
commit e4f8cdc
Show file tree

Hide file tree

Showing 7 changed files with 32 additions and 8 deletions.
diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/CloudInstanceDefaults.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/CloudInstanceDefaults.java
@@ -0,0 +1,8 @@
+package jenkins.plugins.jclouds.compute;
+
+/**
+ * Holds default values for Cloud Instances, like default retention time.
+ */
+final class CloudInstanceDefaults {
+    public static final int DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES = 30;
+}
diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsCloud.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsCloud.java
@@ -60,6 +60,8 @@
 import shaded.com.google.common.collect.Iterables;
 import shaded.com.google.common.io.Closeables;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
+
 /**
  * The JClouds version of the Jenkins Cloud.
  *
@@ -128,10 +130,11 @@ protected Object readResolve() {
     }
 
     /**
-     * Get the retention time, defaulting to 30 minutes.
+     * Get the retention time in minutes or default value from CloudInstanceDefaults if it is zero.
+     * @see CloudInstanceDefaults#DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES
      */
     public int getRetentionTime() {
-        return retentionTime == 0 ? 30 : retentionTime;
+        return retentionTime == 0 ? DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES : retentionTime;
     }
 
     static final Iterable<Module> MODULES = ImmutableSet.<Module>of(new SshjSshClientModule(), new JDKLoggingModule() {

diff --git a/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsSlave.java b/jclouds-plugin/src/main/java/jenkins/plugins/jclouds/compute/JCloudsSlave.java
@@ -20,6 +20,8 @@
 import org.jclouds.domain.LoginCredentials;
 import org.kohsuke.stapler.DataBoundConstructor;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
+
 /**
  * Jenkins Slave node - managed by JClouds.
  *
@@ -134,15 +136,19 @@ public LoginCredentials getCredentials() {
 
     /**
      * Get the retention time for this slave, defaulting to the parent cloud's if not set.
+     * Sometime parent cloud cannot be determined (returns Null as I see), in which case this method will
+     * return default value set in CloudInstanceDefaults.
      *
      * @return overrideTime
+     * @see CloudInstanceDefaults#DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES
      */
     public int getRetentionTime() {
         if (overrideRetentionTime > 0) {
             return overrideRetentionTime;
-        } else {
-            return JCloudsCloud.getByName(cloudName).getRetentionTime();
         }
+
+        JCloudsCloud cloud = JCloudsCloud.getByName(cloudName);
+        return cloud == null ? DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES : cloud.getRetentionTime();
     }
 
     /**

diff --git a/...ugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudInsideJenkinsLiveTest.java b/...ugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudInsideJenkinsLiveTest.java
@@ -9,6 +9,8 @@
 import org.jclouds.ssh.SshKeys;
 import org.jvnet.hudson.test.HudsonTestCase;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
+
 public class JCloudsCloudInsideJenkinsLiveTest extends HudsonTestCase {
 
     private ComputeTestFixture fixture;
@@ -24,7 +26,7 @@ public void setUp() throws Exception {
 
         // TODO: this may need to vary per test
         cloud = new JCloudsCloud(fixture.getProvider() + "-profile", fixture.getProvider(), fixture.getIdentity(), fixture.getCredential(),
-                generatedKeys.get("private"), generatedKeys.get("public"), fixture.getEndpoint(), 1, 30, 600 * 1000, 600 * 1000, null,
+                generatedKeys.get("private"), generatedKeys.get("public"), fixture.getEndpoint(), 1, DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES, 600 * 1000, 600 * 1000, null,
                 Collections.<JCloudsSlaveTemplate>emptyList());
     }
 

diff --git a/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudLiveTest.java b/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudLiveTest.java
@@ -10,6 +10,8 @@
 
 import org.jclouds.ssh.SshKeys;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
+
 public class JCloudsCloudLiveTest extends TestCase {
 
     private ComputeTestFixture fixture;
@@ -25,7 +27,7 @@ public void setUp() throws Exception {
 
         // TODO: this may need to vary per test
         cloud = new JCloudsCloud(fixture.getProvider() + "-profile", fixture.getProvider(), fixture.getIdentity(), fixture.getCredential(),
-                generatedKeys.get("private"), generatedKeys.get("public"), fixture.getEndpoint(), 1, 30, 600 * 1000, 600 * 1000, null,
+                generatedKeys.get("private"), generatedKeys.get("public"), fixture.getEndpoint(), 1, DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES, 600 * 1000, 600 * 1000, null,
                 Collections.<JCloudsSlaveTemplate>emptyList());
     }
 

diff --git a/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudTest.java b/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsCloudTest.java
@@ -1,5 +1,6 @@
 package jenkins.plugins.jclouds.compute;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
@@ -56,7 +57,7 @@ public void testConfigurationUI() throws Exception {
     @Test
     public void testConfigRoundtrip() throws Exception {
 
-        JCloudsCloud original = new JCloudsCloud("aws-profile", "aws-ec2", "identity", "credential", "privateKey", "publicKey", "endPointUrl", 1, 30,
+        JCloudsCloud original = new JCloudsCloud("aws-profile", "aws-ec2", "identity", "credential", "privateKey", "publicKey", "endPointUrl", 1, DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES,
                 600 * 1000, 600 * 1000, null, Collections.<JCloudsSlaveTemplate>emptyList());
 
         j.getInstance().clouds.add(original);

diff --git a/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsSlaveTemplateTest.java b/jclouds-plugin/src/test/java/jenkins/plugins/jclouds/compute/JCloudsSlaveTemplateTest.java
@@ -5,6 +5,8 @@
 
 import org.jvnet.hudson.test.HudsonTestCase;
 
+import static jenkins.plugins.jclouds.compute.CloudInstanceDefaults.DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES;
+
 /**
  * @author Vijay Kiran
  */
@@ -20,7 +22,7 @@ public void testConfigRoundtrip() throws Exception {
         List<JCloudsSlaveTemplate> templates = new ArrayList<JCloudsSlaveTemplate>();
         templates.add(originalTemplate);
 
-        JCloudsCloud originalCloud = new JCloudsCloud("aws-profile", "aws-ec2", "identity", "credential", "privateKey", "publicKey", "endPointUrl", 1, 30,
+        JCloudsCloud originalCloud = new JCloudsCloud("aws-profile", "aws-ec2", "identity", "credential", "privateKey", "publicKey", "endPointUrl", 1, DEFAULT_INSTANCE_RETENTION_TIME_IN_MINUTES,
                 600 * 1000, 600 * 1000, null, templates);
 
         hudson.clouds.add(originalCloud);