Merge pull request #64 from abayer/jenkins-40226

[FIXED JENKINS-40226] Make sure non-within-stage failures are real.
jenkinsci · Dec 12, 2016 · 9bc4f82 · 9bc4f82
2 parents f1574cf + 3cdc103
commit 9bc4f82
Show file tree

Hide file tree

Showing 9 changed files with 169 additions and 63 deletions.
diff --git a/pipeline-model-definition/pom.xml b/pipeline-model-definition/pom.xml
@@ -71,11 +71,6 @@
       <artifactId>workflow-step-api</artifactId>
       <version>2.5</version>
     </dependency>
-    <dependency>
-      <groupId>org.jenkins-ci.plugins.workflow</groupId>
-      <artifactId>workflow-api</artifactId>
-      <version>2.5</version>
-    </dependency>
     <dependency>
       <groupId>org.jenkins-ci.plugins.workflow</groupId>
       <artifactId>workflow-cps</artifactId>

diff --git a/...el-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy b/...el-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy
@@ -259,7 +259,7 @@ public class Utils {
                 tagsAction = new TagsAction()
                 tagsAction.addTag(tagName, tagValue)
                 currentNode.addAction(tagsAction)
-            } else {
+            } else if (tagsAction.getTagValue(tagName) == null) {
                 tagsAction.addTag(tagName, tagValue)
                 currentNode.save()
             }

diff --git a/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy b/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy
@@ -61,67 +61,79 @@ public class ModelInterpreter implements Serializable {
         Throwable firstError
 
         if (root != null) {
-            executeProperties(root)
-
-            // Entire build, including notifications, runs in the withEnv.
-            withEnvBlock(root.getEnvVars()) {
-                inWrappers(root.wrappers) {
-                    // Stage execution and post-build actions run in try/catch blocks, so we still run post-build actions
-                    // even if the build fails.
-                    // We save the caught error, if any, for throwing at the end of the build.
-                    inDeclarativeAgent(root.agent) {
-                        withCredentialsBlock(root.getEnvCredentials()) {
-                            toolsBlock(root.agent, root.tools) {
-                                // If we have an agent and script.scm isn't null, run checkout scm
-                                if (root.agent.hasAgent() && Utils.hasScmContext(script)) {
-                                    script.stage(SyntheticStageNames.checkout()) {
-                                        Utils.markSyntheticStage(SyntheticStageNames.checkout(), Utils.getSyntheticStageMetadata().pre)
-                                        script.checkout script.scm
+            boolean postBuildRun = false
+
+            try {
+                executeProperties(root)
+
+                // Entire build, including notifications, runs in the withEnv.
+                withEnvBlock(root.getEnvVars()) {
+                    inWrappers(root.wrappers) {
+                        // Stage execution and post-build actions run in try/catch blocks, so we still run post-build actions
+                        // even if the build fails.
+                        // We save the caught error, if any, for throwing at the end of the build.
+                        inDeclarativeAgent(root.agent) {
+                            withCredentialsBlock(root.getEnvCredentials()) {
+                                toolsBlock(root.agent, root.tools) {
+                                    // If we have an agent and script.scm isn't null, run checkout scm
+                                    if (root.agent.hasAgent() && Utils.hasScmContext(script)) {
+                                        script.stage(SyntheticStageNames.checkout()) {
+                                            Utils.markSyntheticStage(SyntheticStageNames.checkout(), Utils.getSyntheticStageMetadata().pre)
+                                            script.checkout script.scm
+                                        }
                                     }
-                                }
 
-                                for (int i = 0; i < root.stages.getStages().size(); i++) {
-                                    Stage thisStage = root.stages.getStages().get(i)
-                                    try {
-                                        script.stage(thisStage.name) {
-                                            if (firstError == null) {
-                                                withEnvBlock(thisStage.getEnvVars()) {
-                                                    if (thisStage.when == null || setUpDelegate(thisStage.when.closure)) {
-                                                        inDeclarativeAgent(thisStage.agent) {
-                                                            withCredentialsBlock(thisStage.getEnvCredentials()) {
-                                                                toolsBlock(thisStage.agent ?: root.agent, thisStage.tools) {
-                                                                    // Execute the actual stage and potential post-stage actions
-                                                                    executeSingleStage(root, thisStage)
+                                    for (int i = 0; i < root.stages.getStages().size(); i++) {
+                                        Stage thisStage = root.stages.getStages().get(i)
+                                        try {
+                                            script.stage(thisStage.name) {
+                                                if (firstError == null) {
+                                                    withEnvBlock(thisStage.getEnvVars()) {
+                                                        if (thisStage.when == null || setUpDelegate(thisStage.when.closure)) {
+                                                            inDeclarativeAgent(thisStage.agent) {
+                                                                withCredentialsBlock(thisStage.getEnvCredentials()) {
+                                                                    toolsBlock(thisStage.agent ?: root.agent, thisStage.tools) {
+                                                                        // Execute the actual stage and potential post-stage actions
+                                                                        executeSingleStage(root, thisStage)
+                                                                    }
                                                                 }
                                                             }
+                                                        } else {
+                                                            Utils.markStageSkippedForConditional(thisStage.name)
                                                         }
-                                                    } else {
-                                                        Utils.markStageSkippedForConditional(thisStage.name)
                                                     }
+                                                } else {
+                                                    Utils.markStageSkippedForFailure(thisStage.name)
                                                 }
-                                            }else {
-                                                Utils.markStageSkippedForFailure(thisStage.name)
-                                                }
+                                            }
+                                        } catch (Exception e) {
+                                            script.getProperty("currentBuild").result = Result.FAILURE
+                                            Utils.markStageFailedAndContinued(thisStage.name)
+                                            if (firstError == null) {
+                                                firstError = e
+                                            }
                                         }
+                                    }
+
+                                    // Execute post-build actions now that we've finished all stages.
+                                    try {
+                                        postBuildRun = true
+                                        executePostBuild(root)
                                     } catch (Exception e) {
                                         if (firstError == null) {
                                             firstError = e
                                         }
                                     }
                                 }
-
-                                // Execute post-build actions now that we've finished all stages.
-                                try {
-                                    executePostBuild(root)
-                                } catch (Exception e) {
-                                    if (firstError == null) {
-                                        firstError = e
-                                    }
-                                }
                             }
                         }
                     }
                 }
+            } finally {
+                // If we hit an exception somewhere *before* we got to stages, we still need to do post-build tasks.
+                if (!postBuildRun) {
+                    executePostBuild(root)
+                }
             }
             if (firstError != null) {
                 throw firstError
@@ -368,7 +380,6 @@ public class ModelInterpreter implements Serializable {
             catchRequiredContextForNode(thisStage.agent ?: root.agent) {
                 if (postClosures.size() > 0) {
                     script.echo("Post stage")
-                    //TODO should this be a nested stage instead?
                     try {
                         for (int ni = 0; ni < postClosures.size(); ni++) {
                             setUpDelegate(postClosures.get(ni))

diff --git a/...sci/plugins/pipeline/modeldefinition/agent/impl/DockerPipelineFromDockerfileScript.groovy b/...sci/plugins/pipeline/modeldefinition/agent/impl/DockerPipelineFromDockerfileScript.groovy
@@ -25,6 +25,7 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
+import hudson.model.Result
 import org.jenkinsci.plugins.pipeline.modeldefinition.SyntheticStageNames
 import org.jenkinsci.plugins.pipeline.modeldefinition.Utils
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgent
@@ -48,14 +49,30 @@ public class DockerPipelineFromDockerfileScript extends DeclarativeAgentScript {
             if (!Utils.withinAStage()) {
                 script.stage(SyntheticStageNames.agentSetup()) {
                     Utils.markSyntheticStage(SyntheticStageNames.agentSetup(), Utils.getSyntheticStageMetadata().pre)
-                    buildImage().call()
+                    try {
+                        buildImage().call()
+                    } catch (Exception e) {
+                        script.getProperty("currentBuild").result = Result.FAILURE
+                        Utils.markStageFailedAndContinued(SyntheticStageNames.agentSetup())
+                        throw e
+                    }
                 }
             } else {
-                buildImage().call()
+                try {
+                    buildImage().call()
+                } catch (Exception e) {
+                    script.getProperty("currentBuild").result = Result.FAILURE
+                    throw e
+                }
+            }
+            try {
+                img.inside(declarativeAgent.dockerArgs, {
+                    body.call()
+                })
+            } catch (Exception e) {
+                script.getProperty("currentBuild").result = Result.FAILURE
+                throw e
             }
-            img.inside(declarativeAgent.dockerArgs, {
-                body.call()
-            })
 
         }
     }

diff --git a/...ces/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/DockerPipelineScript.groovy b/...ces/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/DockerPipelineScript.groovy
@@ -25,6 +25,7 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
+import hudson.model.Result
 import org.jenkinsci.plugins.pipeline.modeldefinition.SyntheticStageNames
 import org.jenkinsci.plugins.pipeline.modeldefinition.Utils
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgent
@@ -48,12 +49,23 @@ public class DockerPipelineScript extends DeclarativeAgentScript {
             if (!Utils.withinAStage()) {
                 script.stage(SyntheticStageNames.agentSetup()) {
                     Utils.markSyntheticStage(SyntheticStageNames.agentSetup(), Utils.getSyntheticStageMetadata().pre)
-                    script.getProperty("docker").image(declarativeAgent.docker).pull()
+                    try {
+                        script.getProperty("docker").image(declarativeAgent.docker).pull()
+                    } catch (Exception e) {
+                        script.getProperty("currentBuild").result = Result.FAILURE
+                        Utils.markStageFailedAndContinued(SyntheticStageNames.agentSetup())
+                        throw e
+                    }
                 }
             }
-            script.getProperty("docker").image(declarativeAgent.docker).inside(declarativeAgent.dockerArgs, {
-                body.call()
-            })
+            try {
+                script.getProperty("docker").image(declarativeAgent.docker).inside(declarativeAgent.dockerArgs, {
+                    body.call()
+                })
+            } catch (Exception e) {
+                script.getProperty("currentBuild").result = Result.FAILURE
+                throw e
+            }
         }
     }
 }
diff --git a/...in/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy b/...in/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy
@@ -25,6 +25,7 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
+import hudson.model.Result
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgent
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript
 import org.jenkinsci.plugins.workflow.cps.CpsScript
@@ -38,8 +39,13 @@ public class LabelScript extends DeclarativeAgentScript {
     @Override
     public Closure run(Closure body) {
         return {
-            script.node(declarativeAgent?.label) {
-                body.call()
+            try {
+                script.node(declarativeAgent?.label) {
+                    body.call()
+                }
+            } catch (Exception e) {
+                script.getProperty("currentBuild").result = Result.FAILURE
+                throw e
             }
         }
     }

diff --git a/...ition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java b/...ition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java
@@ -321,8 +321,8 @@ public void noToolSyntheticStage() throws Exception {
     @Test
     public void skippedStagesForFailure() throws Exception {
         WorkflowRun b = expect(Result.FAILURE, "skippedStagesForFailure")
-                .logContains("[Pipeline] { (foo)", "hello")
-                .logNotContains("I will be skipped", "I also will be skipped")
+                .logContains("[Pipeline] { (foo)", "hello", "I have failed")
+                .logNotContains("I will be skipped", "I also will be skipped", "I have succeeded")
                 .go();
 
         assertTrue(b.getExecution().getCauseOfFailure() != null);
@@ -339,6 +339,16 @@ public void skippedStagesForFailure() throws Exception {
         assertNotNull(scanner.findFirstMatch(heads, stageStatusPredicate("baz", Utils.getStageStatusMetadata().getSkippedForFailure())));
     }
 
+    @Issue("JENKINS-40226")
+    @Test
+    public void failureBeforeStages() throws Exception {
+        // This should fail whether we've got Docker available or not. Hopefully.
+        expect(Result.FAILURE, "failureBeforeStages")
+                .logContains("Dockerfile failed")
+                .logNotContains("This should never happen")
+                .go();
+    }
+
     private Predicate<FlowNode> syntheticStagePredicate(String stageName,
                                                         String context) {
         return stageTagPredicate(stageName, Utils.getSyntheticStageMetadata().getTagName(), context);

diff --git a/pipeline-model-definition/src/test/resources/failureBeforeStages.groovy b/pipeline-model-definition/src/test/resources/failureBeforeStages.groovy
@@ -0,0 +1,46 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2016, 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 dockerfile:true, dockerArgs:"-v /tmp:/tmp -p 8000:8000"
+    stages {
+        stage("foo") {
+            steps {
+                sh 'cat /hi-there'
+                sh 'echo "The answer is 42"'
+            }
+        }
+    }
+    post {
+        failure {
+            echo "Dockerfile failed"
+        }
+        success {
+            echo "This should never happen"
+        }
+    }
+}
+
+
+
diff --git a/pipeline-model-definition/src/test/resources/skippedStagesForFailure.groovy b/pipeline-model-definition/src/test/resources/skippedStagesForFailure.groovy
@@ -43,6 +43,15 @@ pipeline {
             }
         }
     }
+
+    post {
+        failure {
+            echo "I have failed"
+        }
+        success {
+            echo "I have succeeded"
+        }
+    }
 }