fix(pipeline_template): Multiple fixes from integration suite (spinna…

…ker#1616)

* fix(pipeline_template): Correctly handle multiple stage injects

* fix(pipeline_template): Fix module rendering in partials

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransform.java

@@ -193,7 +193,7 @@ private static List<StageDefinition> createGraph(List<StageDefinition> stages) {
 
   private static void injectStages(List<StageDefinition> stages, List<StageDefinition> templateStages) {
     // Using a stream here can cause a ConcurrentModificationException.
-    for (StageDefinition s : stages) {
+    for (StageDefinition s : new ArrayList<>(stages)) {
       if (s.getInject() == null) {
         continue;
       }
@@ -205,22 +205,22 @@ private static void injectStages(List<StageDefinition> stages, List<StageDefinit
 
       if (rule.getFirst() != null && rule.getFirst()) {
         injectFirst(s, templateStages);
-        return;
+        continue;
       }
 
       if (rule.getLast() != null && rule.getLast()) {
         injectLast(s, templateStages);
-        return;
+        continue;
       }
 
       if (rule.getBefore() != null) {
         injectBefore(s, rule.getBefore(), templateStages);
-        return;
+        continue;
       }
 
       if (rule.getAfter() != null) {
         injectAfter(s, rule.getAfter(), templateStages);
-        return;
+        continue;
       }
 
       throw new IllegalTemplateConfigurationException(String.format("stage did not have any valid injections defined (id: %s)", s.getId()));
@@ -273,7 +273,7 @@ private static void injectBefore(StageDefinition stage, List<String> targetIds,
     }
 
     stage.getRequisiteStageRefIds().addAll(targetEdges);
-    allStages.add(targetIndexes.last() + 1, stage);
+    allStages.add(targetIndexes.last(), stage);
 
     Map<String, StageDefinition> graph = allStages
       .stream()

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/model/TemplateModule.java

@@ -15,13 +15,14 @@
  */
 package com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model;
 
+import java.util.ArrayList;
 import java.util.List;
 
 public class TemplateModule implements Identifiable {
 
   private String id;
   private String usage;
-  private List<NamedHashMap> variables;
+  private List<NamedHashMap> variables = new ArrayList<>();
   private Object definition;
 
   @Override

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/JinjaRenderer.java

@@ -39,18 +39,18 @@
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Severity;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.yaml.snakeyaml.parser.ParserException;
 
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 public class JinjaRenderer implements Renderer {
 

orca-pipelinetemplate/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/tags/ModuleTag.java

@@ -46,7 +46,6 @@
 public class ModuleTag implements Tag {
 
   private Renderer renderer;
-
   private ObjectMapper objectMapper;
 
   public ModuleTag(Renderer renderer, ObjectMapper pipelineTemplateObjectMapper) {
@@ -144,8 +143,12 @@ public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) {
       );
     }
 
+    if (rendered instanceof CharSequence) {
+      return (String) rendered;
+    }
+
     try {
-      return new String(objectMapper.writeValueAsBytes(rendered));
+      return objectMapper.writeValueAsString(rendered);
     } catch (JsonProcessingException e) {
       throw TemplateRenderException.fromError(
         new Error()

orca-pipelinetemplate/src/test/groovy/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaIntegrationSpec.groovy

@@ -65,8 +65,14 @@ class V1SchemaIntegrationSpec extends Specification {
 
   @Unroll
   def 'test "#integration.name"'() {
-    expect:
-    assertReflectionEquals(integration.expected, subject.process(integration.toRequest()), ReflectionComparatorMode.IGNORE_DEFAULTS)
+    given:
+    def expected = integration.expected
+
+    when:
+    def result = subject.process(integration.toRequest())
+
+    then:
+    assertReflectionEquals(expected, result, ReflectionComparatorMode.IGNORE_DEFAULTS)
 
     where:
     integration << new IntegrationTestDataProvider().provide()

orca-pipelinetemplate/src/test/groovy/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/tags/ModuleTagSpec.groovy

@@ -58,7 +58,6 @@ class ModuleTagSpec extends Specification {
     def result = renderer.render('{% module myModule myOtherVar=world, subject=testerName, job=trigger.job %}', context)
 
     then:
-    // The ModuleTag outputs JSON
-    result == '"hello world, Mr. Tester Testington. You triggered myJob"'
+    result == 'hello world, Mr. Tester Testington. You triggered myJob'
   }
 }

orca-pipelinetemplate/src/test/resources/integration/v1schema/conditionalStage-config.yml

@@ -0,0 +1,7 @@
+---
+schema: "1"
+pipeline:
+  application: orca
+  variables:
+    includeStage: false
+stages: []

orca-pipelinetemplate/src/test/resources/integration/v1schema/conditionalStage-expected.json

@@ -0,0 +1,9 @@
+{
+  "id": "unknown",
+  "keepWaitingPipelines": false,
+  "limitConcurrent": true,
+  "application": "orca",
+  "name": "Unnamed Execution",
+  "stages": [],
+  "notifications": []
+}

orca-pipelinetemplate/src/test/resources/integration/v1schema/conditionalStage-template.yml

@@ -0,0 +1,15 @@
+---
+schema: "1"
+id: conditionalStage
+metadata:
+  name: Conditional stage test
+  description: Tests conditional expressions
+variables:
+- name: includeStage
+stages:
+- id: wait
+  type: wait
+  config:
+    waitTime: 5
+  when:
+  - "{{ includeStage }}"

orca-pipelinetemplate/src/test/resources/integration/v1schema/modules-config.yml

@@ -0,0 +1,13 @@
+---
+schema: "1"
+pipeline:
+  application: orca
+  variables:
+    regions:
+    - us-east-1
+    - us-west-2
+stages: []
+modules:
+- id: wait
+  definition:
+    foo: 10

orca-pipelinetemplate/src/test/resources/integration/v1schema/modules-expected.json

@@ -0,0 +1,39 @@
+{
+  "id": "unknown",
+  "keepWaitingPipelines": false,
+  "limitConcurrent": true,
+  "application": "orca",
+  "name": "Unnamed Execution",
+  "stages": [
+    {
+      "requisiteStageRefIds": [],
+      "name": "wait",
+      "id": null,
+      "refId": "wait",
+      "type": "wait",
+      "someConfig": {
+        "foo": 10
+      }
+    },
+    {
+      "requisiteStageRefIds": ["wait"],
+      "name": "deploy",
+      "id": null,
+      "refId": "deploy",
+      "type": "deploy",
+      "clusters": [
+        {
+          "provider": "aws",
+          "account": "myAccount",
+          "region": "us-east-1"
+        },
+        {
+          "provider": "aws",
+          "account": "myAccount",
+          "region": "us-west-2"
+        }
+      ]
+    }
+  ],
+  "notifications": []
+}

orca-pipelinetemplate/src/test/resources/integration/v1schema/modules-template.yml

@@ -0,0 +1,38 @@
+---
+schema: "1"
+id: modules
+metadata:
+  name: Modules test
+  description: Tests modules functionality
+variables:
+- name: regions
+  type: list
+stages:
+- id: wait
+  type: wait
+  config:
+    someConfig: "{% module wait %}"
+- id: deploy
+  type: deploy
+  dependsOn:
+  - wait
+  config:
+    clusters: |
+      {% for region in regions %}
+      - {% module deployClusterAws region=region %}
+      {% endfor %}
+
+modules:
+- id: wait
+  usage: Defines config for a wait stage
+  definition:
+    foo: 5
+- id: deployClusterAws
+  usage: Defines a deploy stage cluster using the AWS cloud provider
+  variables:
+  - name: region
+    description: The AWS region to deploy into
+  definition:
+    provider: aws
+    account: myAccount
+    region: "{{ region }}"

orca-pipelinetemplate/src/test/resources/integration/v1schema/partials-config.yml

@@ -0,0 +1,5 @@
+---
+schema: "1"
+pipeline:
+  application: orca
+stages: []

orca-pipelinetemplate/src/test/resources/integration/v1schema/partials-expected.json

@@ -0,0 +1,44 @@
+{
+  "id": "unknown",
+  "keepWaitingPipelines": false,
+  "limitConcurrent": true,
+  "application": "orca",
+  "name": "Unnamed Execution",
+  "stages": [
+    {
+      "requisiteStageRefIds": [],
+      "name": "firstWait",
+      "id": null,
+      "refId": "firstWait",
+      "type": "wait",
+      "waitTime": 5
+    },
+    {
+      "requisiteStageRefIds": ["firstWait"],
+      "name": "Build chrome",
+      "id": null,
+      "refId": "buildChrome.buildTarget",
+      "type": "jenkins",
+      "group": "buildBrowser: buildChrome",
+      "foo": "bar"
+    },
+    {
+      "requisiteStageRefIds": ["buildChrome.buildTarget"],
+      "name": "Publish chrome",
+      "id": null,
+      "refId": "buildChrome.publishTarget",
+      "type": "jenkins",
+      "group": "buildBrowser: buildChrome",
+      "baz": "bang"
+    },
+    {
+      "requisiteStageRefIds": ["buildChrome.publishTarget"],
+      "name": "finalWait",
+      "id": null,
+      "refId": "finalWait",
+      "type": "wait",
+      "waitTime": 5
+    }
+  ],
+  "notifications": []
+}

orca-pipelinetemplate/src/test/resources/integration/v1schema/partials-template.yml

@@ -0,0 +1,43 @@
+---
+schema: "1"
+id: partials
+metadata:
+  name: Partials test
+  description: Tests partials spec
+stages:
+- id: firstWait
+  type: wait
+  config:
+    waitTime: 5
+- id: buildChrome
+  type: partial.buildBrowser
+  dependsOn:
+  - firstWait
+  config:
+    target: chrome
+- id: finalWait
+  type: wait
+  dependsOn:
+  - buildChrome
+  config:
+    waitTime: 5
+
+partials:
+- id: buildBrowser
+  usage: Builds the pipeline artifact targeting the a specified browser.
+  variables:
+  - name: target
+    description: The target browser to build for
+  stages:
+  - id: buildTarget
+    type: jenkins
+    name: Build {{ target }}
+    config:
+      foo: bar
+  - id: publishTarget
+    type: jenkins
+    name: Publish {{ target }}
+    dependsOn:
+    - buildTarget
+    config:
+      baz: bang

orca-pipelinetemplate/src/test/resources/integration/v1schema/partialsAndModules-config.yml

@@ -0,0 +1,5 @@
+---
+schema: "1"
+pipeline:
+  application: orca
+stages: []

orca-pipelinetemplate/src/test/resources/integration/v1schema/partialsAndModules-expected.json

@@ -0,0 +1,35 @@
+{
+  "id": "unknown",
+  "keepWaitingPipelines": false,
+  "limitConcurrent": true,
+  "application": "orca",
+  "name": "Unnamed Execution",
+  "stages": [
+    {
+      "requisiteStageRefIds": [],
+      "name": "firstWait",
+      "id": null,
+      "refId": "firstWait",
+      "type": "wait",
+      "waitTime": 5
+    },
+    {
+      "requisiteStageRefIds": ["firstWait"],
+      "name": "Build chrome",
+      "id": null,
+      "refId": "buildChrome.buildTarget",
+      "type": "jenkins",
+      "group": "buildBrowser: buildChrome",
+      "foo": "We're building what? We're building chrome"
+    },
+    {
+      "requisiteStageRefIds": ["buildChrome.buildTarget"],
+      "name": "finalWait",
+      "id": null,
+      "refId": "finalWait",
+      "type": "wait",
+      "waitTime": 5
+    }
+  ],
+  "notifications": []
+}