[JENKINS-38617] Added missing mvn dependnecies to run those tests acc…

…ordingly. Added the right descfiption and using the right function to discover those system groovy scripts defects
jenkinsci · Feb 1, 2017 · 147c0f7 · 147c0f7
1 parent c63106a
commit 147c0f7
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 5 deletions.
diff --git a/pom.xml b/pom.xml
@@ -96,6 +96,13 @@
       <optional>true</optional>
     </dependency>
 
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>groovy</artifactId>
+      <version>1.10</version>
+      <scope>test</scope>
+    </dependency>
+
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>
       <artifactId>groovy</artifactId>

diff --git a/src/main/java/org/jenkins/ci/plugins/jenkinslint/JenkinsLintAction.java b/src/main/java/org/jenkins/ci/plugins/jenkinslint/JenkinsLintAction.java
@@ -9,6 +9,7 @@
 import org.jenkins.ci.plugins.jenkinslint.check.CleanupWorkspaceChecker;
 import org.jenkins.ci.plugins.jenkinslint.check.GitShallowChecker;
 import org.jenkins.ci.plugins.jenkinslint.check.GradleWrapperChecker;
+import org.jenkins.ci.plugins.jenkinslint.check.GroovySystemExitChecker;
 import org.jenkins.ci.plugins.jenkinslint.check.HardcodedScriptChecker;
 import org.jenkins.ci.plugins.jenkinslint.check.JavadocChecker;
 import org.jenkins.ci.plugins.jenkinslint.check.JobAssignedLabelChecker;
@@ -69,6 +70,7 @@ public void getData() throws IOException {
         checkList.add(new HardcodedScriptChecker());
         checkList.add(new GradleWrapperChecker());
         checkList.add(new TimeoutChecker());
+        checkList.add(new GroovySystemExitChecker());
 
         slaveCheckList.add(new SlaveDescriptionChecker());
         slaveCheckList.add(new SlaveVersionChecker());

diff --git a/src/main/java/org/jenkins/ci/plugins/jenkinslint/check/GroovySystemExitChecker.java b/src/main/java/org/jenkins/ci/plugins/jenkinslint/check/GroovySystemExitChecker.java
@@ -19,10 +19,9 @@ public class GroovySystemExitChecker extends AbstractCheck {
 
     public GroovySystemExitChecker() {
         super();
-        this.setDescription("By distributing the wrapper with your project, anyone can work with it without needing to " +
-                            "install Gradle beforehand. Even better, users of the build <br/> are guaranteed to use the " +
-                            "version of Gradle that the build was designed to work with. Further details: " +
-                            "<a href=https://docs.gradle.org/current/userguide/gradle_wrapper.html> Gradle Wrapper docs</a>.");
+        this.setDescription("System groovy scripts run in same JVM as Jenkins master, so there's no surprise that System.exit() kills your Jenkins master. " +
+                            "Throwing an exception is definitely better approach how to announce some problem in the script. Further details: " +
+                            "<a href=https://issues.jenkins-ci.org/browse/JENKINS-14023>JENKINS-14023</a>.");
         this.setSeverity("High");
     }
 
@@ -42,6 +41,8 @@ public boolean executeCheck(Item item) {
                 found = isSystemExit(((MatrixProject) item).getBuilders());
             }
 
+        } else {
+            LOG.log(Level.INFO, "Groovy is not installed");
         }
         return found;
     }
@@ -50,8 +51,14 @@ private boolean isSystemExit (List<Builder> builders) {
         boolean status = false;
         if (builders != null && builders.size() > 0 ) {
             for (Builder builder : builders) {
+                // TODO: Reflection to decouple groovy plugin classs dependencies
+
                 if (builder instanceof hudson.plugins.groovy.SystemGroovy) {
-                    if ((( hudson.plugins.groovy.SystemGroovy) builder).getCommand().toLowerCase().contains("system.exit") ) {
+                    hudson.plugins.groovy.SystemGroovy builder1 = (hudson.plugins.groovy.SystemGroovy) builder;
+                    String command = ((hudson.plugins.groovy.StringScriptSource) builder1.getScriptSource()).getCommand();
+                    LOG.log(Level.INFO, "groovy " + command);
+                    // TODO: Parse to search for non comments, otherwise some false positives!
+                    if (command != null && command.toLowerCase().contains("system.exit") ) {
                         status = true;
                     }
                 }