-
Notifications
You must be signed in to change notification settings - Fork 808
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.
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
Ensure target server group after stages are added correctly #1170
Conversation
@@ -43,11 +43,6 @@ class ModifyAwsScalingProcessStage extends TargetServerGroupLinearStageSupport { | |||
} | |||
|
|||
@Override | |||
def <T extends Execution> List<Stage<T>> aroundStages(Stage<T> parentStage) { | |||
return composeTargets(parentStage); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant — it does exactly the same thing as in the superclass
@@ -36,8 +36,6 @@ class DestroyServerGroupStage extends TargetServerGroupLinearStageSupport { | |||
@Override | |||
<T extends Execution<T>> void taskGraph(Stage<T> stage, TaskNode.Builder builder) { | |||
try { | |||
composeTargets(stage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just cruft from long ago. It doesn't need to do this as part of the main stage as it's done in aroundStages
return composeTargets(parentStage) | ||
} | ||
|
||
@Memoized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes the method safe to be re-entrant. What could go wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📟
@@ -39,10 +40,11 @@ abstract class TargetServerGroupLinearStageSupport implements StageDefinitionBui | |||
String name = this.type | |||
|
|||
@Override | |||
def <T extends Execution> List<Stage<T>> aroundStages(Stage<T> parentStage) { | |||
def <T extends Execution<T>> List<Stage<T>> aroundStages(Stage<T> parentStage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Groovy and weird generic signature glitches.
Because `aroundStages` is re-entrant but in the case of TargetServerGroupLinearStageSupport it also mutates the original stage context it was losing the additional regions. I would like to not have the method be re-entrant at all but this should fix things for now.
fabulous, lets see what happens! |
Tactical fix for SPIN-2298
Because
aroundStages
is re-entrant but in the case of TargetServerGroupLinearStageSupport it also mutates the original stage context it was losing the additional regions. I would like to not have the method be re-entrant at all but this should fix things for now.