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

Markdown summary table #415

Merged
merged 11 commits into from
Aug 25, 2021
Merged

Conversation

Neuro-HSOC
Copy link
Contributor

@Neuro-HSOC Neuro-HSOC commented Aug 23, 2021

Description

Added support for using markdown format for aggregation summary tables. I introduced the config option summary_table_type with the options ascii and markdown to control the behavior. Within the code I fall back to the default of ascii, if the type is not specified. Therefore there are no breaking changes.

Checklist

Questions or Comments

Honestly, I'm not sure how a unit test for a different aggregation table formatting had to look like. Never done one before. I have tested the code by deploying it to my local ElastAlert instance and then raising alerts to my TheHive instance, which supports markdown for the alert styling and the alerts are properly formatted in there.

If you have any advise how to approach such a unit test, I'm happy to try to come up with something. 😄

Added documentation for the configuration options related to the
markdown summary table. These options include:

 - summary_table_type (Defaults to ascii)

 - summary_prefix

 - summary_suffix

Also fixed one too many newline chars at the end of the markdown table.
Added a short description of the possible summary_table_types to the
documentation of the aggregation parameter.
@jertel
Copy link
Owner

jertel commented Aug 23, 2021

This looks useful! I see a few formating typos in the docs, should be formatting. Thanks for fixing that preview/suffix bug - I guess no one uses that feature, or just chose to put up with the suffix not showing up. As far as unit testing goes, take a look at alerts_test.py. This has several unit tests already and I suspect you can get started by using one of them as a template. You'd need to build out an expected string with your markdown contents, and then compare that to the one returned by the get_aggregation_summary_text() method. See how far you get and someone here should be able to help you if you get stuck.

Fixed formating vs formatting typo.
@Neuro-HSOC
Copy link
Contributor Author

Fixed the formating vs formatting typos. The suffix/prefix thing might not be used widely, because as far as I can tell it was never part of the documentation. Likely only people who stare at code know it.

Thanks for the unit test pointer. Will look into this over the next days.

Added unit tests to check the new markdown style aggregation summary
table, the old and now default text table and the summary table
prefix and suffix values.
Added a newline after a non-empty summary_prefix to ensure there is a
linebreak between the set summary_prefix and the hardcoded
'Aggregation resulted in the following data...' table header.
@Neuro-HSOC
Copy link
Contributor Author

Neuro-HSOC commented Aug 25, 2021

Added a unit test for the changes. Based on that I spotted and fixed a small bug in the usage of summary_prefix. If you would set a summary_prefix value, it would have resulted in a header like this:

This is the prefixAggregation resulted in the following data for summary_table_fields ==> ['field', 'abc', 'count']:

+-------------+----------------+-------+
|    field    |      abc       | count |
+=============+================+=======+
| field_value | abc from match | 3     |
+-------------+----------------+-------+
| field_value | cde from match | 2     |
+-------------+----------------+-------+

Now I ensure a newline, if there is a value for summary_prefix set:

This is the prefix
Aggregation resulted in the following data for summary_table_fields ==> ['field', 'abc', 'count']:

It could be a worthwhile discussion though, whether this "Aggregation resulted in..." should probably just become the default prefix value and should be replaced by a custom prefix, if set, instead of having both.

jertel
jertel previously approved these changes Aug 25, 2021
Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the unit tests and fixing the prefix newline issue.

@jertel jertel merged commit 50e591c into jertel:master Aug 25, 2021
@Neuro-HSOC Neuro-HSOC deleted the markdown-summary-table branch August 25, 2021 11:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants