-
Notifications
You must be signed in to change notification settings - Fork 42
Bg stop reorder #1761
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
base: master
Are you sure you want to change the base?
Bg stop reorder #1761
Conversation
dd016cb to
7150540
Compare
Yavor16
left a comment
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.
Can you add also unit test for PrepareToStopDependentModulesStep
...ss/src/main/java/org/cloudfoundry/multiapps/controller/process/steps/IncrementIndexStep.java
Show resolved
Hide resolved
...va/org/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModuleStep.java
Outdated
Show resolved
Hide resolved
| Module applicationModule = findModuleInDeploymentDescriptor(context, getCurrentModuleToStop(context).getName()); | ||
| context.setVariable(Variables.MODULE_TO_DEPLOY, applicationModule); | ||
| CloudApplicationExtended modifiedApp = getApplicationCloudModelBuilder(context).build(applicationModule, moduleToDeployHelper); | ||
| Map<String, String> calculatedAppEnv = applicationEnvironmentCalculator.calculateNewApplicationEnv(context, modifiedApp); | ||
| modifiedApp = ImmutableCloudApplicationExtended.builder() | ||
| .from(modifiedApp) | ||
| .staging(modifiedApp.getStaging()) | ||
| .routes(getApplicationRoutes(context, modifiedApp)) | ||
| .env(calculatedAppEnv) | ||
| .build(); | ||
| context.setVariable(Variables.APP_TO_PROCESS, modifiedApp); | ||
| return StepPhase.DONE; |
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.
Why are you creating here new APP_TO_PROCESS here and what is the difference from the old one?
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.
APP_TO_PROCESS is set to the dependent app inside the subprocess. This is done due to the fact that ExecuteTaskStep relies on that variable to be set. Otherwise the logic would fail whenever a there's a need for hook executions.
...main/java/org/cloudfoundry/multiapps/controller/process/steps/SyncFlowableStepWithHooks.java
Outdated
Show resolved
Hide resolved
...ain/java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolver.java
Outdated
Show resolved
Hide resolved
...rg/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModuleStepTest.java
Show resolved
Hide resolved
...rg/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModuleStepTest.java
Show resolved
Hide resolved
...st/java/org/cloudfoundry/multiapps/controller/process/steps/StopDependentModuleStepTest.java
Show resolved
Hide resolved
| <sequenceFlow id="sid-0F200B9A-6462-48CD-BA2D-6E6F1CE15D96" sourceRef="deleteIdleRoutesTask" targetRef="shouldManageServiceBroker"></sequenceFlow> | ||
| <sequenceFlow id="skipDeleteIdleRoutesFlow" sourceRef="shouldDeleteIdleRoutes" targetRef="shouldManageServiceBroker"></sequenceFlow> |
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.
Why did you change the location of sequence flows?
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.
Its changed due to the new added steps for stopping dependent modules. These specific flows should be unchanged, they've been re-ordered from the bpmn modeler.
...main/java/org/cloudfoundry/multiapps/controller/process/steps/SyncFlowableStepWithHooks.java
Show resolved
Hide resolved
LMCROSSITXSADEPLOY-3367
d053a35 to
6e242eb
Compare
6e242eb to
2f8b5de
Compare
...controller-process/src/main/java/org/cloudfoundry/multiapps/controller/process/Messages.java
Outdated
Show resolved
Hide resolved
...controller-process/src/main/java/org/cloudfoundry/multiapps/controller/process/Messages.java
Outdated
Show resolved
Hide resolved
...controller-process/src/main/java/org/cloudfoundry/multiapps/controller/process/Messages.java
Outdated
Show resolved
Hide resolved
...ain/java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolver.java
Outdated
Show resolved
Hide resolved
...ain/java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolver.java
Outdated
Show resolved
Hide resolved
...ain/java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolver.java
Show resolved
Hide resolved
| return result; | ||
| } | ||
|
|
||
| private boolean isDependencyAwareStopOrderEnabled(ProcessContext context, DeploymentDescriptor descriptor) { |
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.
all private methods may be declared as static for simplicity
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.
They could be static, but I don’t see a clear simplicity gain. Can you share a specific reason?
...org/cloudfoundry/multiapps/controller/process/steps/BuildApplicationDeployModelStepTest.java
Outdated
Show resolved
Hide resolved
...g/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModulesStepTest.java
Show resolved
Hide resolved
...java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolverTest.java
Show resolved
Hide resolved
...java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolverTest.java
Show resolved
Hide resolved
...java/org/cloudfoundry/multiapps/controller/process/util/DependentModuleStopResolverTest.java
Outdated
Show resolved
Hide resolved
...g/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModulesStepTest.java
Show resolved
Hide resolved
...st/java/org/cloudfoundry/multiapps/controller/process/steps/StopDependentModuleStepTest.java
Show resolved
Hide resolved
| private ApplicationWaitAfterStopHandler waitAfterStopHandler; | ||
|
|
||
| @Mock | ||
| private MtaMetadataParser mtaMetadataParser; |
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.
not used?
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.
getModuleDeterminer requires it to be not null, which is called in the abstract parent SyncFlowableStepWithHooks
| private MtaMetadataParser mtaMetadataParser; | ||
|
|
||
| @Mock | ||
| private HooksPhaseGetter hooksPhaseGetter; |
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.
not used?
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.
Required for SyncFlowableStepWithHooks to not throw null pointer exception.
...rg/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModuleStepTest.java
Outdated
Show resolved
Hide resolved
...rg/cloudfoundry/multiapps/controller/process/steps/PrepareToStopDependentModuleStepTest.java
Outdated
Show resolved
Hide resolved
2f35a2f to
20c0c1b
Compare
|



No description provided.