Merge pull request #264 from abayer/jenkins-43016

[JENKINS-43016] Convert empty string label agent to agent any in JSON
jenkinsci · Jun 1, 2018 · c3dc1ea · c3dc1ea
2 parents 3b74fc7 + a382855
commit c3dc1ea
Show file tree

Hide file tree

Showing 3 changed files with 113 additions and 8 deletions.
diff --git a/...l-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java b/...l-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTAgent.java
@@ -4,6 +4,7 @@
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
 
 import javax.annotation.Nonnull;
+import java.util.Map;
 
 /**
  * Represents what context in which to run the build - i.e., which label to run on, what Docker agent to run in, etc.
@@ -22,19 +23,49 @@ public ModelASTAgent(Object sourceLocation) {
     @Override
     public JSONObject toJSON() {
         final JSONObject j = new JSONObject();
-        j.accumulate("type", agentType.toJSON());
 
-        if (variables != null) {
-            if (variables instanceof ModelASTClosureMap &&
-                    !((ModelASTClosureMap) variables).getVariables().isEmpty()) {
-                j.accumulate("arguments", variables.toJSON());
-            } else if (variables instanceof ModelASTValue) {
-                j.accumulate("argument", variables.toJSON());
+        // Handle JENKINS-43016 - round-trip empty-string label agent into agent any.
+        if (isEmptyStringLabelAgent()) {
+            j.accumulate("type", "any");
+        } else {
+            j.accumulate("type", agentType.toJSON());
+
+            if (variables != null) {
+                if (variables instanceof ModelASTClosureMap &&
+                        !((ModelASTClosureMap) variables).getVariables().isEmpty()) {
+                    j.accumulate("arguments", variables.toJSON());
+                } else if (variables instanceof ModelASTValue) {
+                    j.accumulate("argument", variables.toJSON());
+                }
             }
         }
         return j;
     }
 
+    private boolean isEmptyStringLabelAgent() {
+        if (agentType.getKey().equals("label") || agentType.getKey().equals("node")) {
+            if (variables instanceof ModelASTValue && "".equals(((ModelASTValue) variables).getValue())) {
+                return true;
+            }
+            if (variables instanceof ModelASTClosureMap) {
+                Map<ModelASTKey, ModelASTMethodArg> vars = ((ModelASTClosureMap) variables).getVariables();
+                // Don't actually switch to "agent any" if there are any additional options besides the label.
+                if (vars.size() == 1) {
+                    for (Map.Entry<ModelASTKey,ModelASTMethodArg> entry : vars.entrySet()) {
+                        if (entry.getKey().getKey().equals("label")) {
+                            ModelASTMethodArg argValue = entry.getValue();
+                            if (argValue instanceof ModelASTValue && ((ModelASTValue) argValue).getValue().equals("")) {
+                                return true;
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        return false;
+    }
+
     @Override
     public void validate(@Nonnull ModelValidator validator) {
         validator.validateElement(this);

diff --git a/.../src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java b/.../src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParserTest.java
@@ -35,7 +35,7 @@ public void stageWithoutName() throws Exception {
         assertFalse(msg.contains("Exception")); // we don't want stack trace please
     }
 
-    @Issue("JENKINS-41118")
+    @Issue({"JENKINS-41118","JENKINS-43016"})
     @Test
     public void labelWithOptionsBecomesNode() throws Exception {
         ModelASTPipelineDef origRoot = Converter.urlToPipelineDef(getClass().getResource("/inRelativeCustomWorkspace.groovy"));
@@ -63,6 +63,35 @@ public void labelWithOptionsBecomesNode() throws Exception {
         assertEquals(nodeRoot, newRoot);
     }
 
+    @Issue("JENKINS-43016")
+    @Test
+    public void labelWithEmptyStringBecomesAny() throws Exception {
+        ModelASTPipelineDef origRoot = Converter.urlToPipelineDef(getClass().getResource("/agentLabelEmptyString.groovy"));
+
+        assertNotNull(origRoot);
+
+        JSONObject origJson = origRoot.toJSON();
+        assertNotNull(origJson);
+
+        JSONParser jp = new JSONParser(Converter.jsonTreeFromJSONObject(origJson));
+        ModelASTPipelineDef newRoot = jp.parse();
+
+        assertEquals(getJSONErrorReport(jp, "agentLabelEmptyString"), 0, jp.getErrorCollector().getErrorCount());
+        assertNotNull("Pipeline null for agentLabelEmptyString", newRoot);
+
+        JSONObject anyJson = JSONObject.fromObject(fileContentsFromResources("json/agentAny.json"));
+
+        JSONParser anyParser = new JSONParser(Converter.jsonTreeFromJSONObject(anyJson));
+        ModelASTPipelineDef anyRoot = anyParser.parse();
+
+        assertEquals(getJSONErrorReport(anyParser, "agentAny"),
+                0, anyParser.getErrorCollector().getErrorCount());
+        assertNotNull("Pipeline null for agentAny", anyRoot);
+
+        assertEquals(anyRoot, newRoot);
+
+    }
+
     @Test
     public void librariesDirective() throws Exception {
         ModelASTPipelineDef origRoot = Converter.urlToPipelineDef(getClass().getResource("/librariesDirective.groovy"));

diff --git a/pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy b/pipeline-model-definition/src/test/resources/agentLabelEmptyString.groovy
@@ -0,0 +1,45 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2018, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+pipeline {
+    agent {
+        label ''
+    }
+    stages {
+        stage("foo") {
+            steps {
+                script {
+                    if (isUnix()) {
+                        sh('echo "THIS WORKS"')
+                    } else {
+                        bat('echo "THIS WORKS"')
+                    }
+                }
+            }
+        }
+    }
+}
+
+
+