Skip to content

Commit

Permalink
fix(pipelinetemplate): Fix configuration stage injection & requisiteS…
Browse files Browse the repository at this point in the history 
…tageRefId rendering (#1228)
  • Loading branch information
@robzienert
robzienert committed Mar 15, 2017
1 parent af05eec commit ec5af41
Show file tree
Hide file tree
Showing 10 changed files with 139 additions and 53 deletions.
16 changes: 1 addition & 15 deletions ...java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGenerator.java
View file
Expand Up @@ -18,12 +18,10 @@
import com.netflix.spinnaker.orca.pipelinetemplate.generator.ExecutionGenerator;
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate;
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.PipelineTemplate.Configuration;
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.StageDefinition;
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration;

import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
Expand Down Expand Up @@ -61,24 +59,12 @@ public Map<String, Object> generate(PipelineTemplate template, TemplateConfigura
stage.put("refId", s.getId());
stage.put("type", s.getType());
stage.put("name", s.getName());
stage.put("requisiteStageRefIds", getStageRequisiteIds(s, template.getStages()));
stage.put("requisiteStageRefIds", s.getRequisiteStageRefIds());
stage.putAll(s.getConfig());
return stage;
})
.collect(Collectors.toList()));

return pipeline;
}

private static List<String> getStageRequisiteIds(StageDefinition stage, List<StageDefinition> allStages) {
if (stage.getDependsOn().isEmpty()) {
return Collections.emptyList();
}

return allStages
.stream()
.filter(s -> stage.getDependsOn().contains(s.getId()))
.map(StageDefinition::getId)
.collect(Collectors.toList());
}
}
17 changes: 7 additions & 10 deletions ...innaker/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransform.java
View file
Expand Up @@ -52,18 +52,10 @@ public ConfigStageInjectionTransform(TemplateConfiguration templateConfiguration

@Override
public void visitPipelineTemplate(PipelineTemplate pipelineTemplate) {
addAll(pipelineTemplate);
replaceStages(pipelineTemplate);
injectStages(pipelineTemplate);
}

private void addAll(PipelineTemplate pipelineTemplate) {
List<StageDefinition> templateStages = pipelineTemplate.getStages();
if (templateStages.size() == 0) {
templateStages.addAll(templateConfiguration.getStages());
}
}

private void replaceStages(PipelineTemplate pipelineTemplate) {
List<StageDefinition> templateStages = pipelineTemplate.getStages();
for (StageDefinition confStage : templateConfiguration.getStages()) {
Expand All @@ -77,9 +69,14 @@ private void replaceStages(PipelineTemplate pipelineTemplate) {
}

private void injectStages(PipelineTemplate pipelineTemplate) {
// Create initial graph via dependsOn.
// Create initial graph via dependsOn. We need to include stages that the configuration defines with dependsOn as well
List<StageDefinition> initialStages = pipelineTemplate.getStages();
initialStages.addAll(templateConfiguration.getStages().stream()
.filter(s -> !s.getDependsOn().isEmpty())
.collect(Collectors.toList()));

pipelineTemplate.setStages(
createGraph(pipelineTemplate.getStages())
createGraph(initialStages)
);

// Handle stage injections.
Expand Down
25 changes: 23 additions & 2 deletions ...main/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/model/StageDefinition.java
View file
Expand Up @@ -38,8 +38,8 @@ public class StageDefinition implements Identifiable, Conditional {

public static class InjectionRule {

private Boolean first;
private Boolean last;
private Boolean first = false;
private Boolean last = false;
private String before;
private String after;

Expand Down Expand Up @@ -74,6 +74,27 @@ public String getAfter() {
public void setAfter(String after) {
this.after = after;
}

public boolean hasAny() {
return first || last || before != null || after != null;
}

public boolean hasMany() {
int count = 0;
if (first) {
count += 1;
}
if (last) {
count += 1;
}
if (before != null) {
count += 1;
}
if (after != null) {
count += 1;
}
return count > 1;
}
}

public static class InheritanceControl {
Expand Down
15 changes: 15 additions & 0 deletions .../netflix/spinnaker/orca/pipelinetemplate/v1schema/validator/V1SchemaValidationHelper.java
View file
Expand Up @@ -46,6 +46,21 @@ static void validateStageDefinitions(List<StageDefinition> stageDefinitions, Err
.withLocation(locationFormatter.apply("stages." + stageDefinition.getId()))
);
}

if (stageDefinition.getDependsOn() != null && !stageDefinition.getDependsOn().isEmpty() &&
stageDefinition.getInject() != null && stageDefinition.getInject().hasAny()) {
errors.addError(Error.builder()
.withMessage("A stage cannot have both dependsOn and an inject rule defined simultaneously")
.withLocation(locationFormatter.apply("stages." + stageDefinition.getId()))
);
}

if (stageDefinition.getInject() != null && stageDefinition.getInject().hasMany()) {
errors.addError(Error.builder()
.withMessage("A stage cannot have multiple inject rules defined")
.withLocation(locationFormatter.apply("stages." + stageDefinition.getId()))
);
}
});
}
}
10 changes: 10 additions & 0 deletions ...aker/orca/pipelinetemplate/v1schema/validator/V1TemplateConfigurationSchemaValidator.java
View file
Expand Up @@ -19,6 +19,7 @@
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration.PipelineDefinition;
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 com.netflix.spinnaker.orca.pipelinetemplate.validator.SchemaValidator;
import com.netflix.spinnaker.orca.pipelinetemplate.validator.VersionedSchema;

Expand Down Expand Up @@ -55,6 +56,15 @@ public void validate(VersionedSchema configuration, Errors errors) {

V1SchemaValidationHelper.validateStageDefinitions(config.getStages(), errors, V1TemplateConfigurationSchemaValidator::location);

config.getStages().forEach(s -> {
if ((s.getDependsOn() == null || s.getDependsOn().isEmpty()) && (s.getInject() == null || !s.getInject().hasAny())) {
errors.addError(Error.builder()
.withMessage("A configuration-defined stage should have either dependsOn or an inject rule defined")
.withLocation(location(String.format("stages.%s", s.getId())))
.withSeverity(Severity.WARN));
}
});

// TODO rz - validate required variables are set and of the correct type
}

Expand Down
44 changes: 39 additions & 5 deletions ...m/netflix/spinnaker/orca/pipelinetemplate/PipelineTemplatePipelinePreprocessorSpec.groovy
View file
Expand Up @@ -73,6 +73,23 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {
given:
def request = createTemplateRequest('simple-001.yml', [
regions: ['us-east-1', 'us-west-2']
], [
[
id: 'wait',
type: 'wait',
dependsOn: ['tagImage'],
config: [waitTime: 5]
],
[
id: 'injectedWait',
type: 'wait',
inject: [
first: true
],
config: [
waitTime: 5
]
]
])

when:
Expand All @@ -89,12 +106,20 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {
parallel: true,
notifications: [],
stages: [
[
id: null,
refId: 'injectedWait',
requisiteStageRefIds: [],
type: 'wait',
name: 'injectedWait',
waitTime: 5
],
[
id: null,
refId: 'bake',
type: 'bake',
name: 'Bake',
requisiteStageRefIds: [],
requisiteStageRefIds: ['injectedWait'],
regions: ['us-east-1', 'us-west-2'],
package: 'myapp-package',
baseOs: 'trusty',
Expand All @@ -111,6 +136,14 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {
tags: [
stack: 'test'
]
],
[
id: null,
refId: 'wait',
type: 'wait',
name: 'wait',
requisiteStageRefIds: ['tagImage'],
waitTime: 5
]
]
]
Expand All @@ -119,7 +152,7 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {

def 'should render jackson mapping exceptions'() {
when:
def result = subject.process(createTemplateRequest('invalid-template-001.yml', [:], true))
def result = subject.process(createTemplateRequest('invalid-template-001.yml', [:], [], true))

then:
noExceptionThrown()
Expand All @@ -129,14 +162,14 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {

def 'should not render unknown handlebars identifiers'() {
when:
def result = subject.process(createTemplateRequest('invalid-handlebars-001.yml', [:], true))
def result = subject.process(createTemplateRequest('invalid-handlebars-001.yml', [:], [], true))

then:
noExceptionThrown()
result.stages[0].regions == '{{unknown_identifier}}'
}

Map<String, Object> createTemplateRequest(String templatePath, Map<String, Object> variables = [:], boolean plan = false) {
Map<String, Object> createTemplateRequest(String templatePath, Map<String, Object> variables = [:], List<Map<String, Object>> stages = [:], boolean plan = false) {
return [
type: 'templatedPipeline',
trigger: [
Expand All @@ -154,7 +187,8 @@ class PipelineTemplatePipelinePreprocessorSpec extends Specification {
source: getClass().getResource("/templates/${templatePath}").toURI()
],
variables: variables
]
],
stages: stages
],
plan: plan
]
Expand Down
2 changes: 2 additions & 0 deletions ...om/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGeneratorSpec.groovy
View file
Expand Up @@ -54,6 +54,7 @@ class V1SchemaExecutionGeneratorSpec extends Specification {
new StageDefinition(
id: 'bake',
type: 'bake',
requisiteStageRefIds: [],
config: [
regions: ['us-west-2', 'us-east-1'],
package: 'orca-package',
Expand All @@ -67,6 +68,7 @@ class V1SchemaExecutionGeneratorSpec extends Specification {
id: 'tagImage',
type: 'tagImage',
dependsOn: ['bake'],
requisiteStageRefIds: ['bake'],
config: [tags: [stack: 'test']]
)
]
Expand Down
19 changes: 0 additions & 19 deletions ...r/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransformSpec.groovy
View file
Expand Up @@ -24,25 +24,6 @@ import spock.lang.Unroll

class ConfigStageInjectionTransformSpec extends Specification {

def 'should add all stages from configuration if none in template'() {
given:
PipelineTemplate template = new PipelineTemplate(
stages: []
)

TemplateConfiguration configuration = new TemplateConfiguration(
stages: [
new StageDefinition(id: 's1')
]
)

when:
new ConfigStageInjectionTransform(configuration).visitPipelineTemplate(template)

then:
template.stages*.id == ['s1']
}

def 'should replace stages from configuration into template'() {
given:
PipelineTemplate template = new PipelineTemplate(
Expand Down
8 changes: 6 additions & 2 deletions ...ix/spinnaker/orca/pipelinetemplate/v1schema/validator/V1SchemaValidationHelperSpec.groovy
View file
Expand Up @@ -42,13 +42,17 @@ class V1SchemaValidationHelperSpec extends Specification {
new StageDefinition(),
new StageDefinition(id: 'foo'),
new StageDefinition(id: 'foo', type: 'findImage'),
new StageDefinition(id: 'foo', type: 'findImage', config: [:])
new StageDefinition(id: 'foo', type: 'findImage', config: [:]),
new StageDefinition(id: 'foo', type: 'findImage', config: [:], dependsOn: ['bar'], inject: [first: true]),
new StageDefinition(id: 'foo', type: 'findImage', config: [:], inject: [first: true, last: true])
]
expectedError << [
new Errors.Error(message: 'Stage ID is unset', location: 'namespace:stages'),
new Errors.Error(message: 'Stage is missing type', location: 'namespace:stages.foo'),
new Errors.Error(message: 'Stage configuration is unset', location: 'namespace:stages.foo'),
false
false,
new Errors.Error(message: 'A stage cannot have both dependsOn and an inject rule defined simultaneously', location: 'namespace:stages.foo'),
new Errors.Error(message: 'A stage cannot have multiple inject rules defined', location: 'namespace:stages.foo')
]
}

Expand Down
36 changes: 36 additions & 0 deletions ...rca/pipelinetemplate/v1schema/validator/V1TemplateConfigurationSchemaValidatorSpec.groovy
View file
Expand Up @@ -15,11 +15,13 @@
*/
package com.netflix.spinnaker.orca.pipelinetemplate.v1schema.validator

import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.StageDefinition
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration
import com.netflix.spinnaker.orca.pipelinetemplate.v1schema.model.TemplateConfiguration.PipelineDefinition
import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors
import spock.lang.Specification
import spock.lang.Subject
import spock.lang.Unroll

class V1TemplateConfigurationSchemaValidatorSpec extends Specification {

Expand Down Expand Up @@ -69,4 +71,38 @@ class V1TemplateConfigurationSchemaValidatorSpec extends Specification {
null | true
'myapp' | false
}

@Unroll
def "should require either dependsOn or inject rule"() {
given:
def errors = new Errors()
def templateConfiguration = new TemplateConfiguration(
schema: "1",
pipeline: new PipelineDefinition(application: 'myapp'),
stages: [
new StageDefinition(
id: 'foo',
type: 'foo',
config: [:]
)
]
)

when:
subject.validate(templateConfiguration, errors)

then:
if (hasErrors) {
errors.errors[0].message == "A configuration-defined stage should have either dependsOn or an inject rule defined"
errors.errors[0].location == 'configuration:stages.foo'
} else {
!errors.hasErrors(true)
}

where:
dependsOn | injectFirst | hasErrors
null | true | false
['bar'] | false | false
null | null | true
}
}

0 comments on commit ec5af41

Please sign in to comment.