Merge pull request #4 from svanoort/fix-parallel-JENKINS-38464

Fix use of milestone step outside parallel [JENKINS-38464]
jenkinsci · Oct 5, 2016 · 1c90cd1 · 1c90cd1
2 parents 6295778 + b7492d5
commit 1c90cd1
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 37 deletions.
diff --git a/pom.xml b/pom.xml
@@ -30,13 +30,13 @@
     <repositories>
         <repository>
             <id>repo.jenkins-ci.org</id>
-            <url>http://repo.jenkins-ci.org/public/</url>
+            <url>https://repo.jenkins-ci.org/public/</url>
         </repository>
     </repositories>
     <pluginRepositories>
         <pluginRepository>
             <id>repo.jenkins-ci.org</id>
-            <url>http://repo.jenkins-ci.org/public/</url>
+            <url>https://repo.jenkins-ci.org/public/</url>
         </pluginRepository>
     </pluginRepositories>
     <properties>
@@ -51,7 +51,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-api</artifactId>
-            <version>1.15</version>
+            <version>2.4</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>

diff --git a/src/main/java/org/jenkinsci/plugins/pipeline/milestone/MilestoneStepExecution.java b/src/main/java/org/jenkinsci/plugins/pipeline/milestone/MilestoneStepExecution.java
@@ -26,6 +26,7 @@
 import static java.util.logging.Level.WARNING;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -37,13 +38,16 @@
 
 import javax.annotation.CheckForNull;
 
+import com.google.common.base.Predicate;
 import org.jenkinsci.plugins.workflow.actions.ThreadNameAction;
 import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
 import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
 import org.jenkinsci.plugins.workflow.graph.BlockStartNode;
 import org.jenkinsci.plugins.workflow.graph.FlowGraphWalker;
 import org.jenkinsci.plugins.workflow.graph.FlowNode;
 import org.jenkinsci.plugins.workflow.graph.FlowStartNode;
+import org.jenkinsci.plugins.workflow.graphanalysis.FlowScanningUtils;
+import org.jenkinsci.plugins.workflow.graphanalysis.LinearScanner;
 import org.jenkinsci.plugins.workflow.steps.AbstractSynchronousStepExecution;
 import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
 import org.jenkinsci.plugins.workflow.steps.StepContext;
@@ -85,30 +89,15 @@ public Void run() throws Exception {
      * Gets the next ordinal and throw {@link AbortException} the milestone lives inside a parallel step branch.
      */
     private synchronized int processOrdinal() throws AbortException {
-        // TODO: use FlowNodeSerialWalker when released (if possible)
+        List<FlowNode> heads = node.getExecution().getCurrentHeads();
-        FlowGraphWalker walker = new FlowGraphWalker();
+        if (heads.size() > 1) {  // TA-DA!  We're inside a parallel, which is forbidden.
-        walker.addHead(node);
+            throw new AbortException("Using a milestone step inside parallel is not allowed");
-        Integer previousOrdinal = null;
-        int parallelDetectionEnabled = 0;
-        for (FlowNode n : walker) {
-
-            if (parallelDetectionEnabled <= 0 && n.getAction(ThreadNameAction.class) != null) {
-                throw new AbortException("Using a milestone step inside parallel is not allowed");
-            }
-
-            if (n instanceof BlockEndNode) {
-                parallelDetectionEnabled++;
-            } else if (n instanceof BlockStartNode && !(n instanceof FlowStartNode)) {
-                parallelDetectionEnabled--;
-            }
-
-            OrdinalAction a = n.getAction(OrdinalAction.class);
-            if (a != null) {
-                previousOrdinal = a.ordinal;
-                break;
-            }
         }
 
+        Predicate<FlowNode> ordinalMatcher = FlowScanningUtils.hasActionPredicate(OrdinalAction.class);
+        FlowNode lastOrdinalNode = new LinearScanner().findFirstMatch(heads, ordinalMatcher);
+        Integer previousOrdinal = (lastOrdinalNode != null) ? lastOrdinalNode.getAction(OrdinalAction.class).ordinal : null;
+
         // If step.ordinal is set then use it and check order with the previous one
         // Otherwise use calculated ordinal (previousOrdinal + 1)
         int nextOrdinal = 0;
@@ -242,18 +231,9 @@ private static Integer getLastOrdinalInBuild(FlowExecutionOwner.Executable r) {
         try {
             List<FlowNode> heads = owner.get().getCurrentHeads();
             if (heads.size() == 1) {
-                FlowGraphWalker walker = new FlowGraphWalker();
+                Predicate<FlowNode> ordinalMatcher = FlowScanningUtils.hasActionPredicate(OrdinalAction.class);
-                walker.addHead(heads.get(0));
+                FlowNode lastOrdinalNode = new LinearScanner().findFirstMatch(heads, ordinalMatcher);
-                for (FlowNode n : walker) {
+                return (lastOrdinalNode != null) ? lastOrdinalNode.getAction(OrdinalAction.class).ordinal : null;
-                    OrdinalAction action = n.getAction(OrdinalAction.class);
-                    if (action != null) {
-                        lastMilestoneOrdinal = action.ordinal;
-                        break;
-                    }
-                }
-            } else {
-                LOGGER.log(Level.WARNING, "Trying to get last ordinal for a build still in progress?");
-                return null;
             }
         } catch (IOException e) {
             LOGGER.log(Level.WARNING, "Failed to traverse flow graph to search the last milestone ordinal", e);

diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/milestone/MilestoneStepTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/milestone/MilestoneStepTest.java
@@ -9,6 +9,7 @@
 import org.junit.Test;
 import org.junit.runners.model.Statement;
 import org.jvnet.hudson.test.BuildWatcher;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
 
 import static org.junit.Assert.*;
@@ -177,6 +178,24 @@ public void milestoneNotAllowedInsideParallel() {
         });
     }
 
+    @Issue("JENKINS-38464")
+    @Test
+    public void milestoneAllowedOutsideParallel() {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+                p.setDefinition(new CpsFlowDefinition(
+                        "node {\n" +
+                        "  parallel one: { echo 'First' }, two: { \n" +
+                        "    echo 'In-node'\n" +
+                        "  }\n" +
+                        "  milestone 1\n" +
+                        "}"));
+                story.j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+            }
+        });
+    }
+
     @Test
     public void ordinals() {
         story.addStep(new Statement() {