Review fixes

Author: patbos

pom.xml

@@ -134,12 +134,13 @@
       <groupId>org.jenkins-ci.plugins</groupId>
       <artifactId>credentials-binding</artifactId>
     </dependency>
+
+    <!-- for testing -->
     <dependency>
       <groupId>org.jenkins-ci.plugins.workflow</groupId>
       <artifactId>workflow-job</artifactId>
+      <scope>test</scope>
     </dependency>
-
-    <!-- for testing -->
     <dependency>
       <groupId>org.jenkins-ci.plugins.workflow</groupId>
       <artifactId>workflow-basic-steps</artifactId>

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFolderProperty.java

@@ -10,6 +10,7 @@
 import java.util.stream.Collectors;
 
 import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.model.Job;
 import org.apache.commons.lang.StringUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.DoNotUse;
@@ -99,6 +100,19 @@ private static List<UsagePermission> buildPermissions() {
         return ps;
     }
 
+    @SuppressWarnings({"rawtypes"})
+    public static boolean isAllowed(KubernetesSlave agent, Job job) {
+        ItemGroup parent = job.getParent();
+        Set<String> allowedClouds = new HashSet<>();
+
+        KubernetesCloud targetCloud = agent.getKubernetesCloud();
+        if (targetCloud.isUsageRestricted()) {
+            KubernetesFolderProperty.collectAllowedClouds(allowedClouds, parent);
+            return allowedClouds.contains(targetCloud.name);
+        }
+        return true;
+    }
+
     private static String PREFIX_USAGE_PERMISSION = "usage-permission-";
 
     @Override

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesQueueTaskDispatcher.java

@@ -1,62 +1,34 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
 import hudson.Extension;
-import hudson.model.ItemGroup;
 import hudson.model.Job;
 import hudson.model.Node;
 import hudson.model.Queue;
 import hudson.model.Queue.Task;
 
 import hudson.model.queue.CauseOfBlockage;
 import hudson.model.queue.QueueTaskDispatcher;
-import jenkins.model.Jenkins;
-
-import org.jenkinsci.plugins.workflow.job.WorkflowJob;
-
-import java.util.HashSet;
-import java.util.Set;
 
 @Extension
 @SuppressWarnings({"rawtypes"})
 public class KubernetesQueueTaskDispatcher extends QueueTaskDispatcher {
 
     @Override
-    public CauseOfBlockage canTake(Node node, Queue.Task item) {
+    public CauseOfBlockage canTake(Node node, Queue.BuildableItem item) {
         if (node instanceof KubernetesSlave) {
             KubernetesSlave slave = (KubernetesSlave) node;
-            Task ownerTask = item.getOwnerTask();
-            if (ownerTask instanceof WorkflowJob) {
-                WorkflowJob workflowJob = (WorkflowJob) ownerTask;
-                return check(slave, workflowJob);
-            }
-            if (item instanceof Job) {
-                return check(slave, (Job) item);
+            Task ownerTask = item.task.getOwnerTask();
+            if (!KubernetesFolderProperty.isAllowed(slave, (Job) ownerTask)) {
+                return new KubernetesCloudNotAllowed();
             }
         }
         return null;
     }
 
-    @Override
-    public CauseOfBlockage canTake(Node node, Queue.BuildableItem item) {
-        return this.canTake(node, item.task);
-    }
-
-    private CauseOfBlockage check(KubernetesSlave agent, Job job) {
-        ItemGroup parent = job.getParent();
-        Set<String> allowedClouds = new HashSet<>();
-
-        KubernetesCloud targetCloud = Jenkins.get().clouds.getAll(KubernetesCloud.class)
-                .stream()
-                .filter(cloud -> cloud.name.equals(agent.getCloudName()))
-                .findAny()
-                .orElse(null);
-        if (targetCloud != null && targetCloud.isUsageRestricted()) {
-            KubernetesFolderProperty.collectAllowedClouds(allowedClouds, parent);
-            if (!allowedClouds.contains(targetCloud.name)) {
-                return new CauseOfBlockage.BecauseNodeIsBusy(agent);
-            }
+    public static final class KubernetesCloudNotAllowed extends CauseOfBlockage {
+        @Override
+        public String getShortDescription() {
+            return Messages.KubernetesCloudNotAllowed_Description();
         }
-        return null;
     }
-
 }

src/main/resources/org/csanchez/jenkins/plugins/kubernetes/Messages.properties

@@ -7,4 +7,5 @@ KubernetesFolderProperty.displayName=Kubernetes
 KubernetesSlave.HomeWarning=[WARNING] HOME is set to / in the jnlp container. You may encounter \
     troubles when using tools or ssh client. This usually happens if the uid doesn't have any \
     entry in /etc/passwd. Please add a user to your Dockerfile or set the HOME environment \
-    variable to a valid directory in the pod template definition.
+    variable to a valid directory in the pod template definition.
+KubernetesCloudNotAllowed.Description=Kubernetes cloud is not allowed for folder

src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesQueueTaskDispatcherTest.java

@@ -2,6 +2,10 @@
 
 import com.cloudbees.hudson.plugins.folder.Folder;
 import hudson.model.FreeStyleProject;
+import hudson.model.Project;
+import hudson.model.Queue;
+import hudson.model.Slave;
+import hudson.model.queue.CauseOfBlockage;
 import hudson.slaves.DumbSlave;
 import hudson.slaves.RetentionStrategy;
 import net.sf.json.JSONObject;
@@ -14,8 +18,10 @@
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
+import java.util.ArrayList;
+import java.util.Calendar;
+
+import static org.junit.Assert.*;
 import static org.mockito.Mockito.when;
 
 public class KubernetesQueueTaskDispatcherTest {
@@ -31,8 +37,6 @@ public class KubernetesQueueTaskDispatcherTest {
 
     private Folder folderA;
     private Folder folderB;
-    private KubernetesCloud cloudA;
-    private KubernetesCloud cloudB;
     private KubernetesSlave slaveA;
     private KubernetesSlave slaveB;
 
@@ -42,10 +46,10 @@ public void setUpTwoClouds() throws Exception {
         jenkins.jenkins.add(folderA, "Folder A");
         jenkins.jenkins.add(folderB, "Folder B");
 
-        cloudA = new KubernetesCloud("A");
+        KubernetesCloud cloudA = new KubernetesCloud("A");
         cloudA.setUsageRestricted(true);
 
-        cloudB = new KubernetesCloud("B");
+        KubernetesCloud cloudB = new KubernetesCloud("B");
         cloudB.setUsageRestricted(true);
 
         jenkins.jenkins.clouds.add(cloudA);
@@ -78,10 +82,11 @@ public void checkRestrictedTwoClouds() throws Exception {
         FreeStyleProject projectB = folderB.createProject(FreeStyleProject.class, "buildJob");
         KubernetesQueueTaskDispatcher dispatcher = new KubernetesQueueTaskDispatcher();
 
-        assertNull(dispatcher.canTake(slaveA, projectA));
-        assertNotNull(dispatcher.canTake(slaveB, projectA));
-        assertNotNull(dispatcher.canTake(slaveA, projectB));
-        assertNull(dispatcher.canTake(slaveB, projectB));
+        assertNull(dispatcher.canTake(slaveA, new Queue.BuildableItem(new Queue.WaitingItem(Calendar.getInstance(),
+                projectA, new ArrayList<>()))));
+        assertTrue(canTake(dispatcher, slaveB, projectA) instanceof KubernetesQueueTaskDispatcher.KubernetesCloudNotAllowed);
+        assertTrue(canTake(dispatcher, slaveA, projectB) instanceof KubernetesQueueTaskDispatcher.KubernetesCloudNotAllowed);
+        assertNull(canTake(dispatcher, slaveB, projectB));
     }
 
     @Test
@@ -95,7 +100,7 @@ public void checkNotRestrictedClouds() throws Exception {
         KubernetesQueueTaskDispatcher dispatcher = new KubernetesQueueTaskDispatcher();
         KubernetesSlave slave = new KubernetesSlave("C", new PodTemplate(), "testC", "C", "dockerC", new KubernetesLauncher(), RetentionStrategy.INSTANCE);
 
-        assertNull(dispatcher.canTake(slave, project));
+        assertNull(canTake(dispatcher, slave, project));
     }
 
 
@@ -105,7 +110,7 @@ public void checkDumbSlave() throws Exception {
         FreeStyleProject project = jenkins.createProject(FreeStyleProject.class);
         KubernetesQueueTaskDispatcher dispatcher = new KubernetesQueueTaskDispatcher();
 
-        assertNull(dispatcher.canTake(slave, project));
+        assertNull(canTake(dispatcher, slave, project));
     }
 
     @Test
@@ -116,9 +121,18 @@ public void checkPipelinesRestrictedTwoClouds() throws Exception {
         when(task.getOwnerTask()).thenReturn(job);
         KubernetesQueueTaskDispatcher dispatcher = new KubernetesQueueTaskDispatcher();
 
-        assertNull(dispatcher.canTake(slaveA, task));
-        assertNotNull(dispatcher.canTake(slaveB, task));
+        assertNull(canTake(dispatcher, slaveA, task));
+        assertTrue(canTake(dispatcher, slaveB, task) instanceof KubernetesQueueTaskDispatcher.KubernetesCloudNotAllowed);
+    }
+
+    private CauseOfBlockage canTake(KubernetesQueueTaskDispatcher dispatcher, Slave slave, Project project) {
+        return dispatcher.canTake(slave, new Queue.BuildableItem(new Queue.WaitingItem(Calendar.getInstance(),
+                project, new ArrayList<>())));
     }
 
+    private CauseOfBlockage canTake(KubernetesQueueTaskDispatcher dispatcher, Slave slave, Queue.Task task) {
+        return dispatcher.canTake(slave, new Queue.BuildableItem(new Queue.WaitingItem(Calendar.getInstance(),
+                task, new ArrayList<>())));
+    }
 
 }