-
Notifications
You must be signed in to change notification settings - Fork 3
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
DATAGO-89835: Public DC C-EMA config push #226
Conversation
…different envs with different scripts
…st for sending log messages
…an verify that it really is being updated and there is an orgId
return executionLog; | ||
return null; |
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 always null. IntelliJ recommended this.
private void finalizeAndSendResponse(CommandRequest request) { | ||
request.determineStatus(); | ||
private void finalizeAndSendResponse(CommandRequest requestBO) { | ||
requestBO.determineStatus(); |
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.
Rename from request -> requestBO because it's hard to tell what was part of the messaging layer and what is internal.
"orgId", eventPortalProperties.getOrganizationId(), | ||
"orgId", requestBO.getOrgId(), |
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.
No longer grabbing the org ID from the application properties. It now comes from the mop message.
public String getTopicPrefix(String orgId) { | ||
if (StringUtils.isEmpty(orgId)) { | ||
log.debug("Attempted to get topic prefix with empty orgId. Defaulting to application properties org ID {}", | ||
eventPortalProperties.getOrganizationId()); | ||
orgId = eventPortalProperties.getOrganizationId(); | ||
} | ||
return topicPrefix; | ||
return String.format(TOPIC_PREFIX_FORMAT, | ||
orgId, | ||
eventPortalProperties.getRuntimeAgentId()); | ||
} | ||
|
||
public String getTopicPrefix() { | ||
return getTopicPrefix(null); | ||
} |
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.
There's several places where we call this to do things like set the config push command subscription and set the topic to send responses to. It's used for scan, config push, and heartbeat. To make this PR low touch, I've only altered anything to do with config push. That's the only case that sends in an orgId right now.
Maybe scan would want to use this as well? cc @rudraneel-chakraborty
Map<String, String> topicDetails = new HashMap<>(); | ||
topicDetails.put("orgId", eventPortalProperties.getOrganizationId()); | ||
topicDetails.put("orgId", organizationId); | ||
topicDetails.put("runtimeAgentId", eventPortalProperties.getRuntimeAgentId()); | ||
topicDetails.put("messagingServiceId", messagingServiceId); | ||
topicDetails.put(COMMAND_CORRELATION_ID, commandCorrelationId); |
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 should be some kind of object imo but that can be for later.
topicDetails.get(Command.COMMAND_CORRELATION_ID)); | ||
|
||
log.info("Sending config push response to topic: {}", topicString); |
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.
What do we think about logging the topic? It helped with debugging but I wonder if it's too much. I'd like to see a lot of these be debug logs tbh.
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.
I don't think we should log the topics, specially in info level
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.
I've removed this one and kept the subscription one but lowered that one to debug.
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.
Discussed comments with cameron
What is the purpose of this change?
Config push must be able to be performed by several different organizations sharing an EMA.
How was this change implemented?
Use the orgId from the mop message instead of getting it from the app properties to support multi tenancy better.
How was this change tested?
Manually by running the EMA locally and verifying in the debugger that the org ID was set as expected.
Is there anything the reviewers should focus on/be aware of?
None