Add more spotbugs checks and fix existing issues

pom.xml

@@ -50,6 +50,8 @@
     <no-test-jar>false</no-test-jar>
     <useBeta>true</useBeta>
     <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
+    <spotbugs.effort>Max</spotbugs.effort>
+    <spotbugs.threshold>Low</spotbugs.threshold>
   </properties>
 
   <dependencies>

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

@@ -2,6 +2,7 @@
 
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
 import org.jenkinsci.Symbol;
 import org.kohsuke.stapler.DataBoundConstructor;
@@ -13,6 +14,7 @@
 /**
  * Deprecated, use KeyValueEnvVar
  */
+@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
 @Deprecated
 public class ContainerEnvVar extends KeyValueEnvVar {
 

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

@@ -8,10 +8,14 @@
 
 import java.io.Serializable;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Created by fabricio.leotti on 26/04/17.
  */
+@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
 public class ContainerLivenessProbe extends AbstractDescribableImpl<ContainerLivenessProbe> implements Serializable {
+
         private String execArgs;
         private int timeoutSeconds;
         private int initialDelaySeconds;

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

@@ -152,6 +152,9 @@ public KubernetesCloud(String name) {
      * @param source Source Kubernetes cloud implementation
      * @since 0.13
      */
+    @SuppressFBWarnings(value = "MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR", justification = "Problem raised for calling unmarshal. Ignoring the "
+                                                                                             + "warning cause it leads to too many changes, with "
+                                                                                             + "unclear impact.")
     public KubernetesCloud(@NonNull String name, @NonNull KubernetesCloud source) {
         super(name);
         XStream2 xs = new XStream2();

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

@@ -31,6 +31,8 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+
 /**
  * @author Carlos Sanchez carlos@apache.org
  */
@@ -151,14 +153,30 @@ public String toString() {
     }
 
     @Override
+    @NonNull
     public ACL getACL() {
         final ACL base = super.getACL();
-        return new ACL() {
-            @Override
-            public boolean hasPermission(Authentication a, Permission permission) {
-                return permission == Computer.CONFIGURE ? false : base.hasPermission(a,permission);
-            }
-        };
+        return new KubernetesComputerACL(base);
+    }
+
+    /**
+     * Simple static inner class to be used by {@link #getACL()}.
+     * It replaces an anonymous inner class in order to fix
+     * <a href="https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#sic-could-be-refactored-into-a-named-static-inner-class-sic-inner-should-be-static-anon">SIC_INNER_SHOULD_BE_STATIC_ANON</a>.
+     */
+    private static final class KubernetesComputerACL extends ACL {
+
+        private final ACL base;
+
+        public KubernetesComputerACL(final ACL base) {
+            this.base = base;
+        }
+
+        @Override
+        public boolean hasPermission(Authentication a, Permission permission) {
+            return permission == Computer.CONFIGURE ? false : base.hasPermission(a,permission);
+        }
+
     }
 
     public void setLaunching(boolean launching) {

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

@@ -10,6 +10,8 @@
 import java.util.stream.Collectors;
 
 import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import hudson.model.Job;
 
 import org.jenkinsci.Symbol;
@@ -150,27 +152,15 @@ public UsagePermission(String name, boolean granted, boolean inherited) {
             this.inherited = inherited;
         }
 
-        private void setInherited(boolean inherited) {
-            this.inherited = inherited;
-        }
-
         public boolean isInherited() {
             return inherited;
         }
 
-        private void setGranted(boolean granted) {
-            this.granted = granted;
-        }
-
         @SuppressWarnings("unused") // by stapler/jelly
         public boolean isGranted() {
             return granted;
         }
 
-        private void setName(String name) {
-            this.name = name;
-        }
-
         /**
          * Called from Jelly.
          * 
@@ -191,6 +181,8 @@ private static boolean userHasAdministerPermission() {
         return Jenkins.get().hasPermission(Jenkins.ADMINISTER);
     }
 
+
+    @SuppressFBWarnings(value = "UPM_UNCALLED_PRIVATE_METHOD")
     private static boolean isUsageRestrictedKubernetesCloud(Cloud cloud) {
         if (cloud instanceof KubernetesCloud) {
             return ((KubernetesCloud) cloud).isUsageRestricted();

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

@@ -87,7 +87,7 @@ public KubernetesLauncher() {
     }
 
     @Override
-    public boolean isLaunchSupported() {
+    public synchronized boolean isLaunchSupported() {
         return !launched;
     }
 

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

@@ -1,7 +1,9 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
 import java.io.IOException;
 import java.util.HashSet;
+import java.util.Locale;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -13,7 +15,8 @@
 import java.util.logging.Logger;
 
 import edu.umd.cs.findbugs.annotations.CheckForNull;
-import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import hudson.FilePath;
 import hudson.Util;
 import hudson.slaves.SlaveComputer;
@@ -216,6 +219,7 @@ public String getPodName() {
 
     private String remoteFS;
 
+    @SuppressFBWarnings(value = "NM_CONFUSING", justification = "Naming confusion with a getRemoteFs method, but the latter is deprecated.")
     @Override
     public String getRemoteFS() {
         if (remoteFS == null) {
@@ -283,7 +287,7 @@ static String getSlaveName(PodTemplate template) {
             return String.format("%s-%s", DEFAULT_AGENT_PREFIX,  randString);
         }
         // no spaces
-        name = name.replaceAll("[ _]", "-").toLowerCase();
+        name = name.replaceAll("[ _]", "-").toLowerCase(Locale.getDefault());
         // keep it under 63 chars (62 is used to account for the '-')
         name = name.substring(0, Math.min(name.length(), 62 - randString.length()));
         String slaveName = String.format("%s-%s", name, randString);
@@ -570,7 +574,7 @@ public Builder retentionStrategy(RetentionStrategy retentionStrategy) {
             return this;
         }
 
-        private RetentionStrategy determineRetentionStrategy() {
+        private static RetentionStrategy determineRetentionStrategy(@NonNull KubernetesCloud cloud, @NonNull PodTemplate podTemplate) {
             if (podTemplate.getIdleMinutes() == 0) {
                 return new OnceRetentionStrategy(cloud.getRetentionTimeout());
             } else {
@@ -584,6 +588,7 @@ private RetentionStrategy determineRetentionStrategy() {
          * @throws IOException
          * @throws Descriptor.FormException
          */
+        @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "False positive. https://github.com/spotbugs/spotbugs/issues/567")
         public KubernetesSlave build() throws IOException, Descriptor.FormException {
             Validate.notNull(podTemplate);
             Validate.notNull(cloud);
@@ -593,11 +598,11 @@ public KubernetesSlave build() throws IOException, Descriptor.FormException {
                     nodeDescription == null ? podTemplate.getName() : nodeDescription,
                     cloud.name,
                     label == null ? podTemplate.getLabel() : label,
-                    decorateLauncher(computerLauncher == null ? new KubernetesLauncher(cloud.getJenkinsTunnel(), null) : computerLauncher),
-                    retentionStrategy == null ? determineRetentionStrategy() : retentionStrategy);
+                    decorateLauncher(cloud, computerLauncher == null ? new KubernetesLauncher(cloud.getJenkinsTunnel(), null) : computerLauncher),
+                    retentionStrategy == null ? determineRetentionStrategy(cloud, podTemplate) : retentionStrategy);
         }
 
-        private ComputerLauncher decorateLauncher(@NonNull ComputerLauncher launcher) {
+        private ComputerLauncher decorateLauncher(@NonNull KubernetesCloud cloud, @NonNull ComputerLauncher launcher) {
             if (launcher instanceof KubernetesLauncher) {
                 ((KubernetesLauncher) launcher).setWebSocket(cloud.isWebSocket());
             }

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

@@ -1,5 +1,7 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
+import java.util.Locale;
+
 import hudson.model.Label;
 
 public class MetricNames {
@@ -17,7 +19,7 @@ public class MetricNames {
     public static final String PODS_LAUNCHED = PREFIX + ".pods.launched";
 
     public static String metricNameForPodStatus(String status) {
-        String formattedStatus = status == null ? "null" : status.toLowerCase();
+        String formattedStatus = status == null ? "null" : status.toLowerCase(Locale.getDefault());
         return PREFIX + ".pods.launch.status." + formattedStatus;
     }
 

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

@@ -4,6 +4,8 @@
 import org.jenkinsci.plugins.plaincredentials.StringCredentials;
 import org.kohsuke.stapler.DataBoundConstructor;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import com.cloudbees.plugins.credentials.CredentialsScope;
 import com.cloudbees.plugins.credentials.impl.BaseStandardCredentials;
 
@@ -14,6 +16,7 @@
  * @deprecated Use {@link StringCredentials}
  * @author <a href="mailto:andy.block@gmail.com">Andrew Block</a>
  */
+@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
 @Deprecated
 public class OpenShiftTokenCredentialImpl extends BaseStandardCredentials implements TokenProducer {
 

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

@@ -18,13 +18,16 @@
 
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.Descriptor;
 import hudson.model.DescriptorVisibilityFilter;
 import hudson.model.Label;
 import hudson.model.Node;
 import hudson.model.Saveable;
 import hudson.model.TaskListener;
+
 import org.apache.commons.lang.StringUtils;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.pod.retention.PodRetention;
@@ -223,6 +226,9 @@ public PodTemplate(@CheckForNull String id) {
         recomputeLabelDerivedFields();
     }
 
+    @SuppressFBWarnings(value = "MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR", justification = "Warning raised for the call to unmarshal. Ignoring as"
+                                                                                             + " it would require too many changes and uncertain "
+                                                                                             + "impact.")
     public PodTemplate(PodTemplate from) {
         XStream2 xs = new XStream2();
         xs.unmarshal(XStream2.getDefaultDriver().createReader(new StringReader(xs.toXML(from))), this);
@@ -236,6 +242,9 @@ public PodTemplate(String image, List<? extends PodVolume> volumes) {
         this(null, image, volumes);
     }
 
+    @SuppressFBWarnings(value = "MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR", justification =
+            "Warning raised for calling the getContainers method. As " + "this current method is deprecated anyway, it is "
+            + "probably safer to not change it.")
     @Deprecated
     PodTemplate(String name, String image, List<? extends PodVolume> volumes) {
         this(name, volumes, Collections.emptyList());
@@ -322,6 +331,7 @@ public void setRemoteFs(String remoteFs) {
     }
 
     @Deprecated
+    @SuppressFBWarnings(value = "NM_CONFUSING", justification = "Naming confusion with a getRemoteFS method, but the current one is deprecated.")
     public String getRemoteFs() {
         return getFirstContainer().map(ContainerTemplate::getWorkingDir).orElse(ContainerTemplate.DEFAULT_WORKING_DIR);
     }

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

@@ -24,6 +24,8 @@
 import java.io.Serializable;
 import java.util.Collection;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  *  Pod Template Tool Location
  *  This class extends Jenkins DescribableList as implemented in Slave Class. Also implements Serializable interface
@@ -32,6 +34,7 @@
  *
  * @author <a href="mailto:aytuncbeken.ab@gmail.com">Aytunc BEKEN</a>
  */
+@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
 public class PodTemplateToolLocation extends DescribableList<NodeProperty<?>,NodePropertyDescriptor> implements Serializable {
 
     public PodTemplateToolLocation() {}

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

@@ -2,6 +2,7 @@
 
 import java.util.List;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.fabric8.kubernetes.api.model.VolumeMount;
 
 @Deprecated
@@ -11,6 +12,7 @@ public class PodVolumes {
     public static abstract class PodVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume {
     }
 
+    @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
     @Deprecated
     public static class EmptyDirVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume {
 
@@ -24,6 +26,7 @@ protected Object readResolve() {
         }
     }
 
+    @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
     @Deprecated
     public static class SecretVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.SecretVolume {
 
@@ -37,6 +40,7 @@ protected Object readResolve() {
         }
     }
 
+    @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
     @Deprecated
     public static class HostPathVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume {
 
@@ -50,6 +54,7 @@ protected Object readResolve() {
         }
     }
 
+    @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
     @Deprecated
     public static class NfsVolume extends org.csanchez.jenkins.plugins.kubernetes.volumes.NfsVolume {
 
@@ -66,13 +71,15 @@ protected Object readResolve() {
     /**
      * @deprecated Use {@link PodVolume#volumeMountExists(String, List)} instead
      */
+    @Deprecated
     public static boolean volumeMountExists(String path, List<VolumeMount> existingMounts) {
         return PodVolume.volumeMountExists(path, existingMounts);
     }
 
     /**
      * @deprecated Use {@link PodVolume#podVolumeExists(String,List)} instead
      */
+    @Deprecated
     public static boolean podVolumeExists(String path, List<PodVolume> existingVolumes) {
         for (PodVolume podVolume : existingVolumes) {
             if (podVolume.getMountPath().equals(path)) {

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

@@ -10,7 +10,9 @@
 
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
+@SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "Serialization happens exclusively through XStream and not Java Serialization.")
 public class PortMapping extends AbstractDescribableImpl<PortMapping> implements Serializable {
 
     private String name;

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

@@ -1,7 +1,6 @@
 package org.csanchez.jenkins.plugins.kubernetes;
 
 import org.jenkinsci.plugins.kubernetes.credentials.FileSystemServiceAccountCredential;
-import org.jenkinsci.plugins.kubernetes.credentials.TokenProducer;
 import org.kohsuke.stapler.DataBoundConstructor;
 
 import com.cloudbees.plugins.credentials.CredentialsScope;
@@ -14,7 +13,7 @@
  * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
  */
 @Deprecated
-public class ServiceAccountCredential extends FileSystemServiceAccountCredential implements TokenProducer {
+public class ServiceAccountCredential extends FileSystemServiceAccountCredential {
 
     private static final long serialVersionUID = 2739355565227800401L;