Skip to content

Commit

Permalink
fix(core): Remove useless entries from Stage.context (#3359)
Browse files Browse the repository at this point in the history
Fixes spinnaker/spinnaker#5257

The `Stage` constructor was keeping `refId`, `startTimeExpiry` and `requisiteStageRefIds` from the context map argument in its internal context map. This caused issues when a parent stage's context was used to create a child stage, as these entries aren't expected to be inherited. This wasn't a problem for a long time because these values were accidentally removed by the side effect of `StartStageHandler#shouldSkip`. `shouldSkip()` uses `objectMapper.convertValue()` on a stage context expecting to have a cloned map returned. A long standing design/documentation issue with Jackson made `convertValue()` return the input parameter as is instead of converting/cloning it. This made every `Stage` passing through `StartStageHandler.handle()` having their context map indirectly 'corrected' and persisted.
An upcoming upgrade to Spring Boot 2.2 comes with Jackson 2.10 where `objectMapper.convertValue()` has a new behaviour and would make `shouldSkip()` fail to silently 'correct' the stages context map.
FasterXML/jackson-databind#2220

`BakeStageSpec` was validating `context['refId']` on the generated contexts but the contexts aren't Stages yet; once a context is made into a Stage and added to the graph (`graph.add`) it will have its `refId` generated based on the parent refId; which isn't under test in that spec.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
Pierre Delagrave and mergify[bot] committed Jan 21, 2020
1 parent 697722b commit df10b3c
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ class BakeStageSpec extends Specification {
private
static List<Map> expectedContexts(String cloudProvider, String amiSuffix, String... regions) {
return regions.collect {
[cloudProviderType: cloudProvider, amiSuffix: amiSuffix, type: BakeStage.PIPELINE_CONFIG_TYPE, "region": it, name: "Bake in ${it}", refId: "1"]
[cloudProviderType: cloudProvider, amiSuffix: amiSuffix, type: BakeStage.PIPELINE_CONFIG_TYPE, "region": it, name: "Bake in ${it}"]
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ public Stage(Execution execution, String type, String name, Map<String, Object>
this.execution = execution;
this.type = type;
this.name = name;
this.context.putAll(context);

this.refId = (String) context.remove("refId");
this.startTimeExpiry =
Expand All @@ -114,6 +113,8 @@ public Stage(Execution execution, String type, String name, Map<String, Object>
this.requisiteStageRefIds =
Optional.ofNullable((Collection<String>) context.remove("requisiteStageRefIds"))
.orElse(emptySet());

this.context.putAll(context);
}

public Stage(Execution execution, String type, Map<String, Object> context) {
Expand Down

0 comments on commit df10b3c

Please sign in to comment.