[FIXED JENKINS-44497] Allow env vars and such in tools values.

We can't validate those, obviously, but such is life.
jenkinsci · Sep 1, 2017 · 9b0ae5e · 9b0ae5e
1 parent a1fa5a5
commit 9b0ae5e
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 19 deletions.
diff --git a/...inition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/Tools.groovy b/...inition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/Tools.groovy
@@ -40,15 +40,15 @@ import javax.annotation.Nonnull
  * @author Andrew Bayer
  */
 @SuppressFBWarnings(value="SE_NO_SERIALVERSIONID")
-public class Tools extends MappedClosure<String,Tools> implements Serializable {
+public class Tools extends MappedClosure<Closure,Tools> implements Serializable {
 
     private static final Object CACHE_KEY = new Object()
 
     private static final LoadingCache<Object,Map<String,String>> toolTypeCache =
         Utils.generateTypeCache(ToolDescriptor.class, true)
 
     @Whitelisted
-    Tools(Map<String,String> inMap) {
+    Tools(Map<String,Closure> inMap) {
         resultMap = inMap
     }
 
@@ -58,16 +58,13 @@ public class Tools extends MappedClosure<String,Tools> implements Serializable {
      * @return A map of type/name
      */
     @Nonnull
-    public Map<String,Object> mergeToolEntries(@CheckForNull Tools other) {
-        if (other == null) {
-            return getMap()
-        } else {
-            Map<String,Object> mergedMap = [:]
+    public Map<String,Closure> mergeToolEntries(@CheckForNull Tools other) {
+        Map<String,Closure> mergedMap = [:]
+        if (other != null) {
             mergedMap.putAll(other.getMap())
-            mergedMap.putAll(getMap())
-
-            return mergedMap
         }
+        mergedMap.putAll(getMap())
+        return mergedMap
     }
 
     /**

diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/RuntimeASTTransformer.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/RuntimeASTTransformer.groovy
@@ -737,7 +737,11 @@ class RuntimeASTTransformer {
             MapExpression toolsMap = new MapExpression()
             original.tools.each { k, v ->
                 if (v.sourceLocation != null && v.sourceLocation instanceof Expression) {
-                    toolsMap.addMapEntryExpression(constX(k.key), (Expression)v.sourceLocation)
+                    if (v.sourceLocation instanceof ClosureExpression) {
+                        toolsMap.addMapEntryExpression(constX(k.key), (ClosureExpression) v.sourceLocation)
+                    } else {
+                        toolsMap.addMapEntryExpression(constX(k.key), closureX(block(returnS((Expression) v.sourceLocation))))
+                    }
                 }
             }
             return ctorX(ClassHelper.make(Tools.class), args(toolsMap))

diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
@@ -163,8 +163,9 @@ class ModelValidatorImpl implements ModelValidator {
             } else {
                 // Don't bother checking whether the tool exists in this Jenkins master if we know it isn't an allowed tool type.
 
-                // Can't do tools validation without a Jenkins instance, so move on if that's not available.
-                if (Jenkins.getInstance() != null) {
+                // Can't do tools validation without a Jenkins instance, so move on if that's not available, or if the tool value is a
+                // non-literal - we allow users to shoot themselves there.
+                if (Jenkins.getInstance() != null && v.isLiteral()) {
                     // Not bothering with a null check here since we could only get this far if the ToolDescriptor's available in the first place.
                     ToolDescriptor desc = ToolInstallation.all().find { it.getId().equals(Tools.typeForKey(k.key)) }
                     def installer = desc.getInstallations().find { it.name.equals((String) v.value) }

diff --git a/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy b/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy
@@ -356,11 +356,11 @@ public class ModelInterpreter implements Serializable {
      * @return The return of the resulting executed closure
      */
     def toolsBlock(Agent agent, Tools tools, Root root = null, Closure body) {
-        def toolsMap = [:]
+        def toolsMap = new TreeMap<String,Closure>()
         if (tools != null) {
             toolsMap = tools.mergeToolEntries(root?.tools)
         } else if (root?.tools != null) {
-            toolsMap = root.tools.getMap()
+            toolsMap = root.tools.mergeToolEntries(null)
         }
         // If there's no agent, don't install tools in the first place.
         if (agent.hasAgent() && !toolsMap.isEmpty()) {
@@ -384,13 +384,15 @@ public class ModelInterpreter implements Serializable {
         }
     }
 
-    def actualToolsInstall(Map<String,String> toolsMap) {
+    def actualToolsInstall(Map<String,Closure> toolsMap) {
         def toolEnv = []
 
         toolsMap.each { k, v ->
-            String toolPath = script.tool(name: v, type: Tools.typeForKey(k))
+            String toolVer = v.call()
 
-            toolEnv.addAll(script.envVarsForTool(toolId: Tools.typeForKey(k), toolVersion: v))
+            String toolPath = script.tool(name: toolVer, type: Tools.typeForKey(k))
+
+            toolEnv.addAll(script.envVarsForTool(toolId: Tools.typeForKey(k), toolVersion: toolVer))
         }
 
         return toolEnv

diff --git a/...el-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ToolsTest.java b/...el-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ToolsTest.java
@@ -58,7 +58,6 @@ public void simpleTools() throws Exception {
     }
 
     @Issue("JENKINS-44497")
-    @Ignore("Allowing env vars in tool version string breaks validation of version, and env vars aren't interpolated in tool string.")
     @Test
     public void envVarInTools() throws Exception {
         expect("envVarInTools")