Skip to content
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

Add alert_text_header and alert_text_footer options #2096

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnsusek
Copy link
Contributor

When using aggregations, any alert_text gets repeated for every match. So, if you wanted to add some text just at the beginning or end of the aggregation alert, you cannot do this. I added the options alert_text_header and alert_text_footer to allow for this. Now you can add some text to the beginning or end of aggregation alerts.

@Salaander
Copy link
Contributor

Salaander commented Jan 30, 2019

I am actually just preparing a PR that makes it possible to use jinja templating to render the alert messages and I created a new match string formatter that can accept aggregated matches and pass it through jinja so you can have all the display logic abstracted to a template.
The reason I'm writing it here is that when I push it I would like to get some suggestion on how to make it better and conform to be able to accept it. We are now running this version locally and it makes formatting alerts much easier in my opinion. What do you think?

I just found that this actually addresses an open ticket with a few thumbs up :) #750

@johnsusek
Copy link
Contributor Author

@Salaander Using jinja templates to render the alert sounds like a neat feature. And it would make this PR kind of moot. But it would be a bigger change. I'm not sure if they will accept it. You should put in your PR and we'll see what @Qmando thinks about this one, and yours.

@Salaander
Copy link
Contributor

Sure, I agree! I will try to send it either today or tomorrow I still need to write the documentation for it accordingly and add the necessary config changes. I know it is a big change but I hope with some polishing it can go in. I might even need to change how I handle it to be totally backward compatible and yet able to leverage it in the alerts that might benefit from it.

@johnsusek
Copy link
Contributor Author

johnsusek/praeco#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants