DATAGO-84853: cema config push resiliency #211
Conversation
added unit test for TerraformUtils cleanup of config dir
added unit test for TerraformUtils cleanup of config dir
added unit test for TerraformUtils cleanup of config dir
added unit test for TerraformUtils cleanup of config dir
...test/java/com/solace/maas/ep/event/management/agent/plugin/terraform/TerraformUtilsTest.java
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| @Test | ||
| void testBuildTfStateFileDeletionFailureResult(){ |
There was a problem hiding this comment.
Formatting problem: See https://sol-jira.atlassian.net/wiki/spaces/MAASF/pages/480870736/MaaS+Code+Style+Reference to setup for intellij
| @@ -82,6 +82,7 @@ public void reset_mocks() { | |||
| Mockito.reset(terraformClient); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Unnecessary, please remove this line break
.../java/com/solace/maas/ep/event/management/agent/plugin/terraform/manager/TerraformUtils.java
Show resolved
Hide resolved
...ava/com/solace/maas/ep/event/management/agent/plugin/terraform/manager/TerraformManager.java
Show resolved
Hide resolved
.../java/com/solace/maas/ep/event/management/agent/plugin/terraform/manager/TerraformUtils.java
Show resolved
Hide resolved
| private boolean restartSimulated; | ||
|
|
||
| @Override | ||
| public boolean onPhaseChange(InboundMessage message, PersistentMessageHandlerObserverPhase phase) { |
There was a problem hiding this comment.
I have question for this method, I will reach out to you @mynecker
moodiRealist
changed the title
| .logs(List.of()) | ||
| .build()); | ||
| } catch(ApiException e) { | ||
| ApiException apiException = (ApiException) e; |
There was a problem hiding this comment.
This is not needed,
} catch(ApiException apiException) {
log.error("Error executing SEMP delete command: {}", apiException.getResponseBody());
setCommandError(command, apiException);
}
There was a problem hiding this comment.
On second look, basically we can consolidate both exception
} catch (Exception e) {
if (e instanceof ApiException) {
ApiException apiException = (ApiException) e;
log.error("Error executing SEMP delete command: {}", apiException.getResponseBody());
} else {
log.error(ERROR_EXECUTING_COMMAND, e);
}
setCommandError(command, e);
}
There was a problem hiding this comment.
I also have a concern with this log log.error("Error executing SEMP delete command: {}", apiException.getResponseBody());
It can potentially leak sensitive information (such as client username) in plain text
| Validate.isTrue(command.getCommandType().equals(semp), "Command type must be semp"); | ||
| // only delete operation is supported for now and only delete operations are sent from EP | ||
| Validate.isTrue(command.getCommand().equals(SEMP_DELETE_OPERATION), "Command operation must be delete"); | ||
| sempDeleteCommandManager.execute(command, new SempApiProviderImpl(solaceClient)); |
There was a problem hiding this comment.
Instead of creating a new SempApiProviderImpl for each command, we could just change the method signature here as
private void executeSempCommand(Command command, SempApiProviderImplsempApiProvider) {
create a single SempApiProviderImpl on line 138
| }); | ||
| try { | ||
| configPush(requestBO); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Shouldn't we log an error under this catch block?
| @@ -107,28 +109,51 @@ private void handleError(Exception e, CommandRequest requestBO) { | |||
| @SuppressWarnings("PMD") | |||
| public void configPush(CommandRequest request) { | |||
There was a problem hiding this comment.
This method has become quite long, would it be possible to break the content into smaller method calls?
| if (commandLogStreamingProcessorOpt.isPresent()) { | ||
| streamCommandExecutionLogToEpCore(request, command, executionLog); | ||
| } | ||
| if (command.getCommandType() == semp) { |
There was a problem hiding this comment.
I think you can use switch/case statements against this enum instead.
...rc/main/java/com/solace/maas/ep/event/management/agent/command/SempDeleteCommandManager.java
Show resolved
Hide resolved
| try { | ||
| aclProfileApi.deleteMsgVpnAclProfile(request.getMsgVpn(), request.getAclProfileName()); | ||
| } catch (ApiException e) { | ||
| // If the client username does not exist, we don't want to consider it an error |
There was a problem hiding this comment.
I think the method executeDeleteClientUsername was copy pasted few times since this exact comment shows up 5 times in this PR.
That's a good indication of being able to write a helper method that all these 5 execteDelete{something} methods can call.
You can ask github co-pilot to extract the common code from these 5 methods and refactor them for you.
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
I had to add some exclusions on jacoco reports on ep-core PR as well. Please make sure we will pass the line coverage requirements before merging this PR.
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class SempEntityTypeTest { |
There was a problem hiding this comment.
Added some comments, most are minor
One thing to point out: In general I think your intellij is not set up with proper formatting style that we use in ep, you can find the instruction here https://sol-jira.atlassian.net/wiki/spaces/MAASF/pages/480870736/MaaS+Code+Style+Reference
service/application/pom.xml
Outdated
| <source>${java.version}</source> | ||
| <target>${java.version}</target> | ||
| <source>14</source> | ||
| <target>14</target> |
There was a problem hiding this comment.
what was the reason for this change?
There was a problem hiding this comment.
I have the same question, please address this before merging
|
|
||
|
|
||
| /** | ||
| * private void executeSempDeleteCommand(Command command, SempApiProvider sempApiProvider) throws ApiException, JsonProcessingException { |
There was a problem hiding this comment.
Should we keep this commented out code?
There was a problem hiding this comment.
We should not, any code that is not being used MUST be removed
| assertTrue(Files.exists(tempDirectory)); | ||
| //cleanup | ||
| Files.deleteIfExists(tempDirectory); | ||
| assertFalse(Files.exists(tempDirectory)); |
There was a problem hiding this comment.
Temp directories are managed by JUnit, you do not need to clean them up
There was a problem hiding this comment.
in other words, line 35-36 must be removed
| private static Command generateCommand(String tfCommand, String body) { | ||
| return generateCommand(tfCommand, body, false); | ||
| } | ||
|
|
||
| private static Command generateCommand(String tfCommand, String body, Boolean ignoreResult) { | ||
| return generateCommand(tfCommand, body, ignoreResult, | ||
| Map.of("Content-Type", "application/hcl", | ||
| "Content-Encoding", "base64")); | ||
| } | ||
|
|
||
| private static Command generateCommand(String tfCommand, String body, Boolean ignoreResult, Map<String, Object> parameters) { | ||
| return Command.builder() | ||
| .body(Optional.ofNullable(body) | ||
| .map(b -> Base64.getEncoder().encodeToString(b.getBytes(UTF_8))) | ||
| .orElse("")) | ||
| .command(tfCommand) | ||
| .commandType(CommandType.terraform) | ||
| .ignoreResult(ignoreResult) | ||
| .parameters(parameters) | ||
| .build(); | ||
| } | ||
|
|
||
| private static CommandRequest generateCommandRequest(Command commandRequest) { | ||
| return generateCommandRequest(List.of(commandRequest), false); | ||
| } | ||
|
|
||
| private static CommandRequest generateCommandRequest(List<Command> commandRequests, boolean exitOnFailure) { | ||
| return CommandRequest.builder() | ||
| .commandBundles(List.of( | ||
| CommandBundle.builder() | ||
| .executionType(ExecutionType.serial) | ||
| .exitOnFailure(exitOnFailure) | ||
| .commands(commandRequests) | ||
| .build())) | ||
| .commandCorrelationId("234") | ||
| .context("app123") | ||
| .serviceId("ms1234") | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
I am finding it a little difficult to understand the diff here, if I am not wrong, its the exact same code but you pulled it on to the top? @mynecker
| Validate.notNull(request, "request must be provided"); | ||
| Validate.notEmpty(request.getContext(), "context of request must be provided"); | ||
| Validate.notEmpty(request.getServiceId(), "serviceId of request must be provided"); | ||
| Validate.notEmpty(directory, "directory must be provided"); |
There was a problem hiding this comment.
Validations can be outside synchronized block. Its in our interest to keep the lock only to the critical section of the code
There was a problem hiding this comment.
I am approving the PR but please address @moodiRealist comment https://github.com/SolaceProducts/event-management-agent/pull/211/files#r1849067427 before merging
|
SonarQube Quality Gate
|
mynecker
deleted the
mynecker/DATAGO-84853-CEMA-ConfigPush-Resiliency
branch
What is the purpose of this change?
Adding Solace SEMPv2 delete command support to remove dangling resources when executing configPush jobs
How was this change implemented?
adding Java code
How was this change tested?
IT tests and tests in dev environment
Is there anything the reviewers should focus on/be aware of?