Github user rafaelweingartner commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/837#discussion_r108469206
  
    --- Diff: server/src/com/cloud/alert/AlertManagerImpl.java ---
    @@ -767,7 +767,9 @@ public void sendAlert(AlertType alertType, long 
dataCenterId, Long podId, Long c
                     // set up a new alert
                     AlertVO newAlert = new AlertVO();
                     newAlert.setType(alertType.getType());
    -                newAlert.setSubject(subject);
    +                //do not have a seperate column for content.
    +                //appending the message to the subject for now.
    +                newAlert.setSubject(subject+content);
    --- End diff --
    
    I agree with you regarding the time of contributor. I also find it great 
that you documented this and opened a Jira ticket. However, for this specific 
case, I am really not comfortable with the change as it is. As I said before, 
the code at line 772 is opening the gates for unexpected runtime exceptions 
(A.K.A. bugs). If others are willing to take the risk of merging and then later 
dealing with the consequences, I cannot do anything against it. I am only 
pointing at the problem and making it quite clear what I think.
    
    I really do not see any trouble to do things the right way here. It is only 
a matter of creating an alter table SQL that adds a field to a table. Then, you 
have to create this new field in `AlertVO`, and use it; as simple as that. 



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to