[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5154: Fix NPE when no recipients configured for sending alerts

[GitBox <gi...@apache.org>]

GutoVeronezi commented on a change in pull request #5154:
URL: https://github.com/apache/cloudstack/pull/5154#discussion_r659817438



##########
File path: server/src/main/java/com/cloud/alert/AlertManagerImpl.java
##########
@@ -718,6 +718,11 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long c
         mailProps.setContent(content);
         mailProps.setContentType("text/plain");
 
+        if (recipients == null) {
+            s_logger.info("No recipients set in 'alert.email.addresses', skipping sending an alert");

Review comment:
       We could add subject and content to the message, e.g. `... skipping sending alert with subject [%s] and content [%s].`  
     Also, IMHO, I think it would be better to log it as `warning`.

##########
File path: server/src/main/java/com/cloud/alert/AlertManagerImpl.java
##########
@@ -718,6 +718,11 @@ public void sendAlert(AlertType alertType, long dataCenterId, Long podId, Long c
         mailProps.setContent(content);
         mailProps.setContentType("text/plain");
 
+        if (recipients == null) {

Review comment:
       Maybe we could do this validation before instantiating the properties.
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org