Skip to content

Conversation

@vkalapov
Copy link
Contributor

@vkalapov vkalapov commented Jan 9, 2026

No description provided.

@vkalapov vkalapov force-pushed the bg-stop-reorder branch 7 times, most recently from dd016cb to 7150540 Compare January 14, 2026 09:34
Copy link
Contributor

@Yavor16 Yavor16 left a 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

Comment on lines 41 to 47
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;
Copy link
Contributor

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?

Copy link
Contributor Author

@vkalapov vkalapov Jan 19, 2026

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.

completeDeploymentDescriptor.setModules(List.of(module));
context.setVariable(Variables.DEPENDENT_MODULES_TO_STOP, List.of(module));
context.setVariable(Variables.APPS_TO_STOP_INDEX, 0);
context.setVariable(Variables.MTA_MAJOR_SCHEMA_VERSION, 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add unit test for schema version 2. So you can test the validation of the version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason as to why the schema version is set in this test is that when getHandlerFactory() is called it throws an exception if no version is set. Given that the schema version is already validated earlier in DependentModuleStopResolver, adding an additional check here would be redundant.

Comment on lines -120 to -121
<sequenceFlow id="sid-0F200B9A-6462-48CD-BA2D-6E6F1CE15D96" sourceRef="deleteIdleRoutesTask" targetRef="shouldManageServiceBroker"></sequenceFlow>
<sequenceFlow id="skipDeleteIdleRoutesFlow" sourceRef="shouldDeleteIdleRoutes" targetRef="shouldManageServiceBroker"></sequenceFlow>
Copy link
Contributor

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?

Copy link
Contributor Author

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.

LMCROSSITXSADEPLOY-3367
@vkalapov vkalapov force-pushed the bg-stop-reorder branch 2 times, most recently from d053a35 to 6e242eb Compare January 19, 2026 09:13
@sonarqubecloud
Copy link

public static final String ERROR_WHILE_UPDATING_SERVICE_KEYS_METADATA = "Error while updating service keys metadata";
public static final String ERROR_WHILE_POLLING_SERVICE_KEY_OPERATION_0 = "Error while polling service key operation \"{0}\"";
public static final String ERROR_WHILE_STOPPING_DEPENDENT_MODULES = "Failed to stop dependent module with name \"{0}\"";
public static final String ERROR_WHEN_CONFIGURING_STOPPING_OF_DEPENDENT_MODULES = "Failed when configuring the stopping of dependent modules \"{0}\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

All other error messages are combination of ERROR + WHILE, only this is with WHEN, do you think that it can be renamed for consistency?

public static final String ERROR_WHILE_DETERMINING_SERVICE_KEYS_TO_RECREATE = "Error while determining service keys to recreate";
public static final String ERROR_WHILE_UPDATING_SERVICE_KEYS_METADATA = "Error while updating service keys metadata";
public static final String ERROR_WHILE_POLLING_SERVICE_KEY_OPERATION_0 = "Error while polling service key operation \"{0}\"";
public static final String ERROR_WHILE_STOPPING_DEPENDENT_MODULES = "Failed to stop dependent module with name \"{0}\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

The constant name is plural, but the text is not, do you think that it should be aligned?
_DEPENDENT_MODULES -> _DEPENDENT_MODULE?

public static final String SERVICE_INSTANCE_0_PARAMETERS_UPDATE_FAILED_IGNORING_FAILURE = "Service instance: \"{0}\" parameters update failed, ignoring failure...";
public static final String SERVICE_INSTANCE_0_TAGS_UPDATE_FAILED_IGNORING_FAILURE = "Service instance: \"{0}\" tags update failed, ignoring failure...";
public static final String ONLY_FIRST_SERVICE_WILL_BE_CREATED = "Only the first service will be created because the provided 'service-name' fields are duplicated! All other services with the same 'service-name' will be ignored! Duplicated names: {0}";
public static final String SKIPPING_DEPENDENCY_ORDER_STOP = "Skipping stopping modules in dependency-aware order. This feature is reserved only for blue green deployment strategy.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is used for blue-green deployment you may add it to the constant name, you may also consider "available for" instead of "reserved only".

}

private boolean supportsDeployedAfter(Module module, StepLogger logger) {
if (module.getMajorSchemaVersion() >= 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may use DEPLOYED_AFTER_MIN_SCHEMA_VERSION instead of "magic" numbers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants