-
Notifications
You must be signed in to change notification settings - Fork 4
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
[Build-trigger-54] Sending a confirmation email to the person that triggered the build #59
base: main
Are you sure you want to change the base?
Conversation
da9472d
to
63f66bc
Compare
@@ -27,10 +32,22 @@ public class BuildTrigger { | |||
public void triggerBuild(BuildJMSTriggerPayload payloadMessage) { | |||
try (JMSContext context = connectionFactory.createContext(Session.AUTO_ACKNOWLEDGE)) { | |||
context.createProducer().send(context.createTopic(triggerTopic), objectMapper.writeValueAsString(payloadMessage)); | |||
logger.infof("Build message successfully sent to UMB for repository: %s, version: %s", |
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.
-> "Build message sent to UMB..."
@@ -27,10 +32,22 @@ public class BuildTrigger { | |||
public void triggerBuild(BuildJMSTriggerPayload payloadMessage) { | |||
try (JMSContext context = connectionFactory.createContext(Session.AUTO_ACKNOWLEDGE)) { | |||
context.createProducer().send(context.createTopic(triggerTopic), objectMapper.writeValueAsString(payloadMessage)); | |||
logger.infof("Build message successfully sent to UMB for repository: %s, version: %s", | |||
payloadMessage.gitRepo, payloadMessage.projectVersion); | |||
if (payloadMessage.email != null && !payloadMessage.email.isBlank() |
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.
Is the indentation alright for this if clause?
} catch (JMSRuntimeException e) { | ||
logger.errorf("Failed to send build message to UMB for tag: %s. Exception: %s", | ||
payloadMessage.tag, e.getMessage()); | ||
throw new BuildTriggerException("Failed to send build message to UMB due to internal error: " + e.getMessage(), e); |
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.
-> "Failed to send build message to UMB due to: ..."
throw new RuntimeException("Failed to serialize BuildJMSTriggerPayload to JSON: " + e.getMessage(), e); | ||
throw new BuildTriggerException("Failed to serialize BuildJMSTriggerPayload to JSON: " + e.getMessage(), e); | ||
} catch (JMSRuntimeException e) { | ||
logger.errorf("Failed to send build message to UMB for tag: %s. Exception: %s", |
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.
We have two similar loggers, one on L35, saying that the build was sent and the other here, saying that it wasn't sent. Both of them are including different information. Should be unified
Mailer mailer; | ||
|
||
public void sendMail(BuildJMSTriggerPayload payload) { | ||
String subject = "Request to build project: " + payload.gitRepo + ", version: " + payload.projectVersion + " successfully sent"; |
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.
That's a way too long for a subject. I'd go with something like [Build Trigger] Build of <component>, version <version> requested"
|
||
public void sendMail(BuildJMSTriggerPayload payload) { | ||
String subject = "Request to build project: " + payload.gitRepo + ", version: " + payload.projectVersion + " successfully sent"; | ||
String text = "Request to build project: " + payload.gitRepo + ", version: " + payload.projectVersion |
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.
Request to build <component> in version <version> was successfully sent to a productization team using following parameters:
Tag:
Repository:
Version:
Commit:
Streams:
Report any issues or share suggestions using our GitHub Repository
This is an automated Build Trigger message, please do not respond!
.setText(text) | ||
.setTo(List.of(payload.email))); | ||
|
||
logger.infof("Sending mail to %s about successful request to build project.\n Subject: %s\n Text: %s", payload.email, subject, text); |
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 perhaps way too much logging given the fact that we can check the email itself.
logger.infof("Sending mail to %s about successful request to build project", payload.email);
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 moved this to BuildTrigger and modified it a bit because we are not the ones receiving the email. The person who asked for a build is receiving the email and we should get at least a notification about sending this email so we know everything went well. Or what did you mean?
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 meant that the content of the email can be verified either by sending the email to us or said person can redirect the email to us as well
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.
OK, I removed that.
", version='" + projectVersion + '\'' + | ||
", commitSha='" + commitSha + '\'' + | ||
", streams='" + streams + '\'' + | ||
return "BuildJMSTriggerPayload{" + "\n" + |
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.
The email would probably require some formatting method to somewhat prettify the data here
7e52126
to
3fb2fbf
Compare
Mailer mailer; | ||
|
||
public void sendMail(BuildJMSTriggerPayload payload) { | ||
String subject = "[Build Trigger] Build of " + payload.gitRepo + ", version: " + payload.projectVersion + " requested"; |
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.
That's not a component, but a git repo :) The line (as well as the one below) should look like "[Build Trigger] Build of Resteasy, version 3.2.1 requested" as opposed to "[Build Trigger] Build of https://github.com/resteasy/Resteasy/, version 3.2.1 requested"
Or similar formatting, looking at it I'm not a fan of the ",version x.x.x requested" part. Do you have any idea how to change it?
We might want to add project
property for the BuildInfo as we'll be using it here and in the CU creation as well. For now, let's add
String project = payload.gitRepo.substring(payload.gitRepo.lastIndexOf('/') + 1)
…iggered the build
3fb2fbf
to
b0e440f
Compare
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.
LGTM, thanks
resolves #54