fix(pipelinetemplate): Handlebars rendering & variable inheritance is…

…sues (#1230)
spinnaker · Mar 17, 2017 · 0123525 · 0123525
1 parent 0783f94
commit 0123525
Show file tree

Hide file tree

Showing 15 changed files with 224 additions and 22 deletions.
diff --git a/...rc/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/GraphMutator.java b/...rc/main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/GraphMutator.java
@@ -20,6 +20,7 @@
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.ConditionalStanzaTransform;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.ConfigModuleReplacementTransform;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.ConfigStageInjectionTransform;
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.DefaultVariableAssignmentTransform;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.PipelineConfigInheritanceTransform;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.RenderTransform;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform.StageInheritanceControlTransform;
@@ -36,6 +37,7 @@ public class GraphMutator {
   List<PipelineTemplateVisitor> visitors = new ArrayList<>();
 
   public GraphMutator(TemplateConfiguration configuration, Renderer renderer, Registry registry, Map<String, Object> trigger) {
+    visitors.add(new DefaultVariableAssignmentTransform(configuration));
     visitors.add(new ConditionalStanzaTransform(configuration, renderer, trigger));
     visitors.add(new ConfigModuleReplacementTransform(configuration));
     visitors.add(new PipelineConfigInheritanceTransform(configuration));

diff --git a/.../spinnaker/orca/pipelinetemplate/v1schema/graph/transform/ConditionalStanzaTransform.java b/.../spinnaker/orca/pipelinetemplate/v1schema/graph/transform/ConditionalStanzaTransform.java
@@ -44,13 +44,13 @@ public void visitPipelineTemplate(PipelineTemplate pipelineTemplate) {
     trimConditionals(templateConfiguration.getStages(), pipelineTemplate);
   }
 
-  private <T extends Conditional> void trimConditionals(List<T> list, PipelineTemplate template) {
-    if (list == null) {
+  private <T extends Conditional> void trimConditionals(List<T> stages, PipelineTemplate template) {
+    if (stages == null) {
       return;
     }
 
-    for (T el : list) {
-      if (el.getWhen() == null || el.getWhen().size() == 0) {
+    for (T el : stages) {
+      if (el.getWhen() == null || el.getWhen().isEmpty()) {
         continue;
       }
 
@@ -60,7 +60,7 @@ private <T extends Conditional> void trimConditionals(List<T> list, PipelineTemp
       for (String conditional : el.getWhen()) {
         String rendered = renderer.render(conditional, context);
         if (!Boolean.parseBoolean(rendered)) {
-          list.remove(el);
+          stages.remove(el);
           return;
         }
       }

diff --git a/...er/orca/pipelinetemplate/v1schema/graph/transform/DefaultVariableAssignmentTransform.java b/...er/orca/pipelinetemplate/v1schema/graph/transform/DefaultVariableAssignmentTransform.java
@@ -0,0 +1,61 @@
+/*
+ * Copyright 2017 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform;
+
+import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.IllegalTemplateConfigurationException;
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.PipelineTemplateVisitor;
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate;
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate.Variable;
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class DefaultVariableAssignmentTransform implements PipelineTemplateVisitor {
+
+  TemplateConfiguration templateConfiguration;
+
+  public DefaultVariableAssignmentTransform(TemplateConfiguration templateConfiguration) {
+    this.templateConfiguration = templateConfiguration;
+  }
+
+  @Override
+  public void visitPipelineTemplate(PipelineTemplate pipelineTemplate) {
+    if (pipelineTemplate.getVariables() == null || pipelineTemplate.getVariables().isEmpty()) {
+      return;
+    }
+
+    Map<String, Object> configVars = templateConfiguration.getPipeline().getVariables();
+
+    // if the config is missing vars and the template defines a default value, assign those values from the config
+    pipelineTemplate.getVariables().stream()
+      .filter(templateVar -> !configVars.containsKey(templateVar.getName()) && templateVar.hasDefaultValue())
+      .forEach(templateVar -> configVars.put(templateVar.getName(), templateVar.getDefaultValue()));
+
+    List<String> missingVariables = pipelineTemplate.getVariables().stream()
+      .filter(templateVar -> !configVars.containsKey(templateVar.getName()))
+      .map(Variable::getName)
+      .collect(Collectors.toList());
+
+    if (!missingVariables.isEmpty()) {
+      throw new IllegalTemplateConfigurationException("Missing variable values for: " + StringUtils.join(missingVariables, ", "));
+    }
+
+    // TODO rz - validate variable values match the defined variable type
+  }
+}
diff --git a/...com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/RenderTransform.java b/...com/netflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/RenderTransform.java
@@ -18,6 +18,7 @@
 import com.netflix.spectator.api.Registry;
 import com.netflix.spectator.api.Timer;
 import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.IllegalTemplateConfigurationException;
+import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.TemplateRenderException;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.PipelineTemplateVisitor;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate;
 import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.StageDefinition;
@@ -60,6 +61,7 @@ public void visitPipelineTemplate(PipelineTemplate pipelineTemplate) {
 
   private void render(PipelineTemplate template) {
     RenderContext context = new RenderContext(templateConfiguration.getPipeline().getApplication(), template, trigger);
+
     context.putAll(templateConfiguration.getPipeline().getVariables());
 
     // We only render the stages here, whereas modules will be rendered only if used within stages.
@@ -74,7 +76,13 @@ private void renderStages(List<StageDefinition> stages, RenderContext context) {
     }
 
     for (StageDefinition stage : stages) {
-      Object rendered = RenderUtil.deepRender(renderer, stage.getConfig(), context);
+      Object rendered;
+      try {
+        rendered = RenderUtil.deepRender(renderer, stage.getConfig(), context);
+      } catch (TemplateRenderException e) {
+        throw new TemplateRenderException("Failed rendering stage '" + stage.getId() + "': " + e.getMessage());
+      }
+
       if (!(rendered instanceof Map)) {
         throw new IllegalTemplateConfigurationException("A stage's rendered config must be a map");
       }

diff --git a/...ain/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/model/PipelineTemplate.java b/...ain/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/model/PipelineTemplate.java
@@ -72,6 +72,10 @@ public Object getDefaultValue() {
     public void setDefaultValue(Object defaultValue) {
       this.defaultValue = defaultValue;
     }
+
+    public boolean hasDefaultValue() {
+      return defaultValue != null;
+    }
   }
 
   public static class Configuration {

diff --git a/.../java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/HandlebarsRenderer.java b/.../java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/HandlebarsRenderer.java
@@ -63,8 +63,6 @@ public HandlebarsRenderer(ObjectMapper pipelineTemplateObjectMapper) {
 
   @Override
   public String render(String template, RenderContext configuration) {
-    log.debug("rendering '" + template + "'");
-
     Template tmpl;
     try {
       tmpl = handlebars.compileInline(template);
@@ -74,13 +72,26 @@ public String render(String template, RenderContext configuration) {
 
     Context context = Context.newContext(configuration);
 
+    String rendered;
     try {
-      return tmpl.apply(context);
+      rendered = tmpl.apply(context);
     } catch (IOException e) {
+      log.error("Failed rendering template: " + template, e);
       throw new TemplateRenderException("could not apply context to template", e);
     } catch (HandlebarsException e) {
+      log.error("Failed rendering template: " + template, e);
       throw new TemplateRenderException(e.getMessage(), e.getCause());
     }
+
+    // Large handlebar templates can inject a lot of extra whitespace we don't want
+    // and can lead to incorrect (or non-existent) template-object expansion
+    rendered = rendered.trim().replaceAll("\n", "");
+
+    if (!template.equals(rendered)) {
+      log.debug("rendered '" + template + "' -> '" + rendered + "'");
+    }
+
+    return rendered;
   }
 
   @Override

diff --git a/...java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/ModuleHelper.java b/...java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/ModuleHelper.java
@@ -56,7 +56,7 @@ public ModuleHelper(Renderer renderer, ObjectMapper objectMapper) {
 
   @Override
   public String apply(Object context, Options options) throws IOException {
-    isTrue(context instanceof String, "found '%s', expected 'module id'", context);
+    isTrue(context instanceof String, "module: could not find module by given id '%s', or id type is invalid", context);
 
     PipelineTemplate template = options.get("pipelineTemplate");
     if (template == null) {

diff --git a/.../com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/WithMapKeyHelper.java b/.../com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/WithMapKeyHelper.java
@@ -28,10 +28,10 @@ public class WithMapKeyHelper implements Helper<Map<String, Object>> {
 
   @Override
   public Object apply(Map<String, Object> context, Options options) throws IOException {
-    notNull(context, "Map value must not be null");
+    notNull(context, "withMapKey: Map value must not be null");
 
     String key = options.param(0);
-    notNull(key, "A key is required");
+    notNull(key, "withMapKey: A key is required");
 
     if (!context.containsKey(key)) {
       throw new IllegalArgumentException("withObjectKey helper given key that does not exist (key: " + key + ")");

diff --git a/...m/netflix/spinnaker/orca/pipelinetemplate/PipelineTemplatePipelinePreprocessorSpec.groovy b/...m/netflix/spinnaker/orca/pipelinetemplate/PipelineTemplatePipelinePreprocessorSpec.groovy
@@ -27,6 +27,7 @@ import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.render.Renderer
 import org.unitils.reflectionassert.ReflectionComparatorMode
 import spock.lang.Specification
 import spock.lang.Subject
+import spock.lang.Unroll
 
 import static org.unitils.reflectionassert.ReflectionAssert.assertReflectionEquals
 
@@ -169,7 +170,21 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {
     result.stages[0].regions == '{{unknown_identifier}}'
   }
 
-  Map<String, Object> createTemplateRequest(String templatePath, Map<String, Object> variables = [:], List<Map<String, Object>> stages = [:], boolean plan = false) {
+  @Unroll
+  def 'should not render falsy conditional stages'() {
+    when:
+    def result = subject.process(createTemplateRequest('conditional-stages-001.yml', [includeWait: includeWait]))
+
+    then:
+    result.stages*.name == expectedStageNames
+
+    where:
+    includeWait || expectedStageNames
+    true        || ['wait', 'conditionalWait']
+    false       || ['wait']
+  }
+
+  Map<String, Object> createTemplateRequest(String templatePath, Map<String, Object> variables = [:], List<Map<String, Object>> stages = [], boolean plan = false) {
     return [
       type: 'templatedPipeline',
       trigger: [

diff --git a/...r/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransformSpec.groovy b/...r/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransformSpec.groovy
@@ -108,7 +108,7 @@ class ConfigStageInjectionTransformSpec extends Specification {
     def configBuilder = { injectRule ->
       new TemplateConfiguration(
         stages: [
-          new StageDefinition(id: 'injected', type: 'manualJudgement', inject: injectRule)
+          new StageDefinition(id: 'injected', type: 'manualJudgment', inject: injectRule)
         ]
       )
     }

diff --git a/...a/pipelinetemplate/v1schema/graph/transform/DefaultVariableAssignmentTransformSpec.groovy b/...a/pipelinetemplate/v1schema/graph/transform/DefaultVariableAssignmentTransformSpec.groovy
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2017 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.orca.pipelinetemplate.v1schema.graph.transform
+
+import com.netflix.spinnaker.orca.pipelinetemplate.exceptions.IllegalTemplateConfigurationException
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate.Variable
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration
+import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration.PipelineDefinition
+import spock.lang.Specification
+
+class DefaultVariableAssignmentTransformSpec extends Specification {
+
+  def 'should assign default variables if configuration defines none'() {
+    given:
+    def template = new PipelineTemplate(
+      variables: [
+        new Variable(name: 'foo', defaultValue: 'fooDefaultValue'),
+        new Variable(name: 'bar'),
+        new Variable(name: 'baz', defaultValue: 'bazDefaultValue')
+      ]
+    )
+
+    def configuration = new TemplateConfiguration(
+      pipeline: new PipelineDefinition(
+        variables: [
+          bar: 'barValue',
+          baz: 'bazValue'
+        ]
+      )
+    )
+
+    def subject = new DefaultVariableAssignmentTransform(configuration)
+
+    when:
+    subject.visitPipelineTemplate(template)
+
+    then:
+    configuration.pipeline.variables.with {
+      it['foo'] == 'fooDefaultValue'
+      it['bar'] == 'barValue'
+      it['baz'] == 'bazValue'
+    }
+  }
+
+  def 'should throw template configuration exception if required variable is undefined'() {
+    given:
+    def template = new PipelineTemplate(
+      variables: [
+        new Variable(name: 'bar')
+      ]
+    )
+
+    def configuration = new TemplateConfiguration(
+      pipeline: new PipelineDefinition(
+        variables: [:]
+      )
+    )
+
+    def subject = new DefaultVariableAssignmentTransform(configuration)
+
+    when:
+    subject.visitPipelineTemplate(template)
+
+    then:
+    thrown(IllegalTemplateConfigurationException)
+  }
+}
diff --git a/...tflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/RenderTransformSpec.groovy b/...tflix/spinnaker/orca/pipelinetemplate/v1schema/graph/transform/RenderTransformSpec.groovy
@@ -51,7 +51,8 @@ class RenderTransformSpec extends Specification {
       id: 'deployToPrestaging',
       schema: '1',
       variables: [
-        new Variable(name: 'regions', type: 'list')
+        new Variable(name: 'regions', type: 'list'),
+        new Variable(name: 'slackChannel', type: 'string', defaultValue: '#det')
       ],
       configuration: new Configuration(
         triggers: [
@@ -77,22 +78,22 @@ class RenderTransformSpec extends Specification {
           ]
         ),
         new StageDefinition(
-          id: 'manualJudgement',
-          type: 'manualJudgement',
+          id: 'manualjudgment',
+          type: 'manualjudgment',
           dependsOn: ['findImage'],
           config: [
             propagateAuthentication: true,
             notifications: [
               type: 'slack',
-              channel: '#det',
+              channel: '{{slackChannel}}',
               when: ['awaiting']
             ]
           ]
         ),
         new StageDefinition(
           id: 'deploy',
           type: 'deploy',
-          dependsOn: ['manualJudgement'],
+          dependsOn: ['manualJudgment'],
           config: [
             clusters: '[{{#each regions}}{{module "deployCluster" region=this}}{{#unless @last}},{{/unless}}{{/each}}]'
           ]

diff --git a/...com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/HandlebarsRendererSpec.groovy b/...com/netflix/spinnaker/orca/pipelinetemplate/v1schema/render/HandlebarsRendererSpec.groovy
@@ -50,7 +50,8 @@ class HandlebarsRendererSpec extends Specification {
     '1.1'                 || Double         | 1.1
     'true'                || Boolean        | true
     '{{ stringVar }}'     || String         | 'myStringValue'
-    '''\
+    '''
+
 [
   {{#each regions}}
   "{{ this }}"{{#unless @last}},{{/unless}}

diff --git a/...netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/ConditionHelperSpec.groovy b/...netflix/spinnaker/orca/pipelinetemplate/v1schema/render/helper/ConditionHelperSpec.groovy
@@ -70,8 +70,8 @@ class ConditionHelperSpec extends Specification {
     t.cause.getClass() == IllegalArgumentException
 
     where:
-    template                            | context
-    '{{contains value "something"}}'    | [value: 1]
+    template                             | context
+    '{{contains value "something"}}'     | [value: 1]
     '{{containsKey value "something"}}'  | [value: 1]
   }