[JENKINS-44285] Tool Location overwrites are not preserved

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

@@ -512,7 +512,7 @@ public PodTemplate getTemplate(@CheckForNull Label label) {
      * @param podTemplate the pod template to unwrap.
      * @return the unwrapped pod template
      */
-    public PodTemplate getUnwrappedTemplate(PodTemplate podTemplate) {
+    public PodTemplate getUnwrappedTemplate(PodTemplate podTemplate) throws IOException {
         return PodTemplateUtils.unwrap(podTemplate, getDefaultsProviderTemplate(), getAllTemplates());
     }
 

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

@@ -1,5 +1,6 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
+import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -9,9 +10,14 @@
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-
 import javax.annotation.Nonnull;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import hudson.model.*;
+import hudson.slaves.NodeProperty;
+import hudson.slaves.NodePropertyDescriptor;
+import hudson.util.DescribableList;
 import org.apache.commons.lang.StringUtils;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
@@ -21,19 +27,11 @@
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
-
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
-
 import hudson.Extension;
 import hudson.Util;
-import hudson.model.AbstractDescribableImpl;
-import hudson.model.Descriptor;
-import hudson.model.DescriptorVisibilityFilter;
-import hudson.model.Label;
-import hudson.model.Node;
 import hudson.model.labels.LabelAtom;
-import hudson.tools.ToolLocationNodeProperty;
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import jenkins.model.Jenkins;
@@ -43,7 +41,7 @@
  *
  * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
  */
-public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements Serializable {
+public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements Serializable, Saveable {
 
     private static final long serialVersionUID = 3285310269140845583L;
 
@@ -112,15 +110,15 @@ public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements
 
     private List<PodImagePullSecret> imagePullSecrets = new ArrayList<PodImagePullSecret>();
 
-    private transient List<ToolLocationNodeProperty> nodeProperties;
+    private PodTemplateToolLocation nodeProperties;
 
     private String yaml;
 
     @DataBoundConstructor
     public PodTemplate() {
     }
 
-    public PodTemplate(PodTemplate from) {
+    public PodTemplate(PodTemplate from) throws IOException {
         this.setAnnotations(from.getAnnotations());
         this.setContainers(from.getContainers());
         this.setImagePullSecrets(from.getImagePullSecrets());
@@ -137,6 +135,7 @@ public PodTemplate(PodTemplate from) {
         this.setVolumes(from.getVolumes());
         this.setWorkspaceVolume(from.getWorkspaceVolume());
         this.setYaml(from.getYaml());
+        this.setNodeProperties(from.getNodeProperties());
     }
 
     @Deprecated
@@ -486,18 +485,19 @@ public void setImagePullSecrets(List<PodImagePullSecret> imagePullSecrets) {
     }
 
     @DataBoundSetter
-    public void setNodeProperties(List<ToolLocationNodeProperty> nodeProperties){
-        this.nodeProperties = nodeProperties;
+    public void setNodeProperties(List<? extends NodeProperty<?>> properties)  {
+        this.getNodeProperties().clear();
+        this.getNodeProperties().addAll(properties);
     }
 
-    @Nonnull
-    public List<ToolLocationNodeProperty> getNodeProperties(){
-        if (nodeProperties == null) {
-            return Collections.emptyList();
-        }
+    @NonNull
+    public PodTemplateToolLocation getNodeProperties(){
+        if( this.nodeProperties == null)
+            this.nodeProperties = new PodTemplateToolLocation(this);
         return nodeProperties;
     }
 
+
     @Deprecated
     public String getResourceRequestMemory() {
         return getFirstContainer().map(ContainerTemplate::getResourceRequestMemory).orElse(null);
@@ -671,6 +671,12 @@ private void optionalField(StringBuilder builder, String label, String value) {
         }
     }
 
+    /**
+     * Empty implementation of Saveable interface. This interface is used for DescribableList implementation
+     */
+    @Override
+    public void save()  { }
+
     @Extension
     public static class DescriptorImpl extends Descriptor<PodTemplate> {
 

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

@@ -0,0 +1,36 @@
+package org.csanchez.jenkins.plugins.kubernetes;
+
+import hudson.model.Saveable;
+import hudson.slaves.NodeProperty;
+import hudson.slaves.NodePropertyDescriptor;
+import hudson.util.DescribableList;
+
+import java.io.Serializable;
+import java.util.Collection;
+
+/**
+ *  Pod Template Tool Location
+ *  This class extends Jenkins DescribableList as implemented in Slave Class. Also implements Serializable interface
+ *  for PodTemplate Class.
+ *  Using DescribableList is not possible directly in PodTemplate because DescribableList is not Serializable.
+ *
+ * @author <a href="mailto:aytuncbeken.ab@gmail.com">Aytunc BEKEN</a>
+ */
+public class PodTemplateToolLocation extends DescribableList<NodeProperty<?>,NodePropertyDescriptor> implements Serializable {
+
+    public PodTemplateToolLocation() {}
+
+
+    public PodTemplateToolLocation(DescribableList.Owner owner) {
+        super(owner);
+    }
+
+    public PodTemplateToolLocation(Saveable owner) {
+        super(owner);
+    }
+
+    public PodTemplateToolLocation(Saveable owner, Collection<? extends NodeProperty<?>> initialList) {
+        super(owner,initialList);
+    }
+
+}

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

@@ -28,6 +28,10 @@
 import javax.annotation.Nonnull;
 
 import org.apache.commons.lang.StringUtils;
+import hudson.slaves.NodeProperty;
+import hudson.slaves.NodePropertyDescriptor;
+import hudson.util.DescribableList;
+import jenkins.model.Jenkins;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.workspace.WorkspaceVolume;
@@ -305,8 +309,7 @@ public static PodTemplate combine(PodTemplate parent, PodTemplate template) {
         WorkspaceVolume workspaceVolume = template.isCustomWorkspaceVolumeEnabled() && template.getWorkspaceVolume() != null ? template.getWorkspaceVolume() : parent.getWorkspaceVolume();
 
         //Tool location node properties
-        List<ToolLocationNodeProperty> toolLocationNodeProperties = new ArrayList<>();
-        toolLocationNodeProperties.addAll(parent.getNodeProperties());
+        PodTemplateToolLocation toolLocationNodeProperties = parent.getNodeProperties();
         toolLocationNodeProperties.addAll(template.getNodeProperties());
 
         PodTemplate podTemplate = new PodTemplate();

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

@@ -1,8 +1,6 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
 import java.util.Arrays;
-import java.util.Collections;
-
 import jenkins.model.JenkinsLocationConfiguration;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;

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

@@ -30,6 +30,8 @@
 import java.util.Arrays;
 import java.util.List;
 
+import hudson.slaves.NodePropertyDescriptor;
+import hudson.util.DescribableList;
 import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume;
@@ -45,6 +47,11 @@
 import com.cloudbees.plugins.credentials.SystemCredentialsProvider;
 import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
 
+import hudson.model.Node;
+import hudson.plugins.git.GitTool;
+import hudson.slaves.NodeProperty;
+import hudson.tools.ToolLocationNodeProperty;
+import hudson.tools.ToolLocationNodeProperty.ToolLocation;
 import hudson.util.Secret;
 
 /**
@@ -88,6 +95,21 @@ public void upgradeFrom_0_12() throws Exception {
                 new KeyValueEnvVar("pod_b_key", "pod_b_value")), templates.get(0).getEnvVars());
     }
 
+    @Test
+    @LocalData()
+    public void upgradeFrom_0_10() throws Exception {
+        List<PodTemplate> templates = cloud.getTemplates();
+        PodTemplate template = templates.get(0);
+        DescribableList<NodeProperty<?>,NodePropertyDescriptor> nodeProperties = template.getNodeProperties();
+        assertEquals(1, nodeProperties.size());
+        ToolLocationNodeProperty property = (ToolLocationNodeProperty) nodeProperties.get(0);
+        assertEquals(1, property.getLocations().size());
+        ToolLocation location = property.getLocations().get(0);
+        assertEquals("Default", location.getName());
+        assertEquals("/custom/path", location.getHome());
+        assertEquals(GitTool.class, location.getType().clazz);
+    }
+
     @Test
     @LocalData()
     public void upgradeFrom_0_8() throws Exception {

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

@@ -30,10 +30,14 @@
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
+import hudson.model.Node;
+import hudson.slaves.NodeProperty;
+import hudson.tools.ToolLocationNodeProperty;
 import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.model.SecretEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume;
@@ -215,7 +219,7 @@ public void shouldUnwrapParent() {
     }
 
     @Test
-    public void shouldDropNoDataWhenIdentical() {
+    public void shouldDropNoDataWhenIdentical()  {
         PodTemplate podTemplate = new PodTemplate();
         podTemplate.setName("Name");
         podTemplate.setNamespace("NameSpace");
@@ -253,7 +257,7 @@ public void shouldDropNoDataWhenIdentical() {
     }
 
     @Test
-    public void shouldUnwrapMultipleParents() {
+    public void shouldUnwrapMultipleParents()  {
         PodTemplate parent = new PodTemplate();
         parent.setName("parent");
         parent.setLabel("parent");
@@ -293,7 +297,7 @@ public void shouldUnwrapMultipleParents() {
     }
 
     @Test
-    public void shouldCombineAllPodKeyValueEnvVars() {
+    public void shouldCombineAllPodKeyValueEnvVars()  {
         PodTemplate template1 = new PodTemplate();
         KeyValueEnvVar podEnvVar1 = new KeyValueEnvVar("key-1", "value-1");
         template1.setEnvVars(singletonList(podEnvVar1));
@@ -309,7 +313,7 @@ public void shouldCombineAllPodKeyValueEnvVars() {
     }
 
     @Test
-    public void shouldFilterOutNullOrEmptyPodKeyValueEnvVars() {
+    public void shouldFilterOutNullOrEmptyPodKeyValueEnvVars()  {
         PodTemplate template1 = new PodTemplate();
         KeyValueEnvVar podEnvVar1 = new KeyValueEnvVar("", "value-1");
         template1.setEnvVars(singletonList(podEnvVar1));
@@ -340,7 +344,7 @@ public void shouldCombineAllPodSecretEnvVars() {
     }
 
     @Test
-    public void shouldFilterOutNullOrEmptyPodSecretEnvVars() {
+    public void shouldFilterOutNullOrEmptyPodSecretEnvVars()  {
         PodTemplate template1 = new PodTemplate();
         SecretEnvVar podSecretEnvVar1 = new SecretEnvVar("", "secret-1", "secret-key-1");
         template1.setEnvVars(singletonList(podSecretEnvVar1));
@@ -438,7 +442,7 @@ public void shouldFilterOutEnvFromSourcesWithNullOrEmptyKey() {
     }
 
     @Test
-    public void shouldCombineAllMounts() {
+    public void shouldCombineAllMounts()  {
         PodTemplate template1 = new PodTemplate();
         HostPathVolume hostPathVolume1 = new HostPathVolume("/host/mnt1", "/container/mnt1");
         HostPathVolume hostPathVolume2 = new HostPathVolume("/host/mnt2", "/container/mnt2");
@@ -614,4 +618,23 @@ public void testValidateLabelTooLong() {
         assertFalse(validateLabel("1234567890123456789012345678901234567890123456789012345678901234"));
         assertFalse(validateLabel("abcdefghijklmnopqrstuwxyzabcdefghijklmnopqrstuwxyzabcdefghijklmn"));
     }
+
+    @Test
+    public void shouldCombineAllToolLocations() {
+
+        PodTemplate podTemplate1 = new PodTemplate();
+        List<ToolLocationNodeProperty> nodeProperties1 = new ArrayList<>();
+        nodeProperties1.add(new ToolLocationNodeProperty(new ToolLocationNodeProperty.ToolLocation("toolKey1@Test","toolHome1")));
+        podTemplate1.setNodeProperties(nodeProperties1);
+
+        PodTemplate podTemplate2 = new PodTemplate();
+        List<ToolLocationNodeProperty> nodeProperties2 = new ArrayList<>();
+        nodeProperties2.add(new ToolLocationNodeProperty(new ToolLocationNodeProperty.ToolLocation("toolKey2@Test","toolHome2")));
+        podTemplate2.setNodeProperties(nodeProperties2);
+
+        PodTemplate result = combine(podTemplate1,podTemplate2);
+
+        assertThat(result.getNodeProperties(), hasItems(nodeProperties1.get(0),nodeProperties2.get(0)));
+
+    }
 }