Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): Remove useless entries from Stage.context #3359

Merged
merged 4 commits into from
Jan 21, 2020
Merged

fix(core): Remove useless entries from Stage.context #3359

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81668ff
fix(core): Remove useless entries from `Stage.context`
Jan 9, 2020
46a5968
Merge branch 'master' into stage_context_fix
Jan 16, 2020
2e6adde
Merge branch 'master' into stage_context_fix
Jan 20, 2020
175d9fa
Merge branch 'master' into stage_context_fix
mergify[bot] Jan 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion orca-bakery/src/test/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStageSpec.groovy
View file
Open in desktop
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}"]
}
}
}
3 changes: 2 additions & 1 deletion orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java
View file
Open in desktop
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());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole function seems suspect to me (i've looked into this before but ran into some issues trying to fix it - don't recall what they were)
the context this function receives as the param is passed by ref so we actually modify the context from the caller which is a bit messed up.
but AFAIR there were no calls to this that mattered so i sort of put it aside...
anyway, the change LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is passed by reference, shouldn't a stage copy the context? Would we need to do a deep copy or would a copy of the map only be sufficient?

this.context.putAll(context);
}

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