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.

Sign up for GitHub

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

Jump to bottom

VictorOps: Set state_message and entity_display_name from rule #329

Merged
merged 4 commits into from Jul 21, 2021
Merged

VictorOps: Set state_message and entity_display_name from rule #329

merged 4 commits into from Jul 21, 2021

Conversation

ChristophShyper
Copy link
Contributor

This change adds all data gathered from ElasticSearch to VictorOps's payload, which then can be used easily with transmogrifier.

If victorops_entity_display_name is not set then entity_display_name is taking value of alert_subject. Or randomized by VictorOps.

@ChristophShyper
VictorOps: Set state_message and entity_display_name from rule
99d70b7 
This change adds all data gathered from ElasticSearch to VictorOps's payload, which then can be used easily with transmogrifier.

If `victorops_entity_display_name` is not set then `entity_display_name` is taking value of `alert_subject`. Or randomized by VictorOps.
@ChristophShyper ChristophShyper mentioned this pull request Jul 5, 2021
Open
@nsano-rururu
Copy link
Collaborator

It has changed its original behavior and will not be merged unless you modify the test code.

@ChristophShyper
Copy link
Contributor Author

I noticed that. Maybe I'll have a look some other day to correct it.

ChristophShyper added a commit to ChristophShyper/elastalert2 that referenced this pull request Jul 20, 2021
@ChristophShyper
Updated tests in victorops_test.py to accommodate new fields passed…
81a1f8b 
… to alert payload.

* Needed by jertel#329
@ChristophShyper
Copy link
Contributor Author

Hi @nsano-rururu I've updated the tests for the new payload content.

@nsano-rururu
Copy link
Collaborator

Isn't it necessary to modify the document?
https://github.com/jertel/elastalert2/blob/master/docs/source/ruletypes.rst#splunk-on-call-formerly-victorops

ChristophShyper added a commit to ChristophShyper/elastalert2 that referenced this pull request Jul 20, 2021
@ChristophShyper
Updated tests in victorops_test.py to accommodate new fields passed…
6c30527 
… to alert payload.

* Needed by jertel#329
@ChristophShyper
Copy link
Contributor Author

You're right. I've added there also information about the default behaviour.

ChristophShyper added a commit to ChristophShyper/elastalert2 that referenced this pull request Jul 20, 2021
@ChristophShyper
Updated tests in victorops_test.py to accommodate new fields passed…
59e8fbc 
… to alert payload.

* Needed by jertel#329
@nsano-rururu
Copy link
Collaborator

I think I need to add some test code if victorops_entity_display_name is not set

ChristophShyper added a commit to ChristophShyper/elastalert2 that referenced this pull request Jul 20, 2021
@ChristophShyper
Updated tests in victorops_test.py to accommodate new fields passed…
30b118a 
… to alert payload.

* Added test for a default `entity_display_name`.
* Needed by jertel#329
@ChristophShyper
Copy link
Contributor Author

Added it as well now. Sorry, it's 2 AM and I'm working for 18 hours already so my mind is shutting down...

@jertel jertel requested a review from nsano-rururu July 21, 2021 02:30
nsano-rururu
nsano-rururu reviewed Jul 21, 2021
View reviewed changes
Copy link
Collaborator

@nsano-rururu nsano-rururu left a comment

Choose a reason for hiding this comment

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

Please add your changes to 2.x.x New features in change logs.
https://github.com/jertel/elastalert2/blob/master/CHANGELOG.md

@ChristophShyper
Updated tests in victorops_test.py to accommodate new fields passed…
0968fef 
… to alert payload.

* Fixed unit tests for VictorOps.
* Added test for a default `entity_display_name`.
* Updated CHANGELOG.md.
* Needed by jertel#329
@ChristophShyper
Copy link
Contributor Author

Done. Added as breaking changes, since the subject could change.

@nsano-rururu nsano-rururu self-requested a review July 21, 2021 11:20
nsano-rururu
nsano-rururu previously approved these changes Jul 21, 2021
View reviewed changes
@nsano-rururu
Copy link
Collaborator

@jertel

The review is over. Is there anything else I can point out?

@jertel
Copy link
Owner

jertel commented Jul 21, 2021

This was added to the Breaking Changes section of the changelog. But is this really a breaking change? For users that have VictorOps in use today, how will upgrading to the next version break their alerts?

@ChristophShyper
Copy link
Contributor Author

ChristophShyper commented Jul 21, 2021 •
edited

Depending on transmogrifier configuration, if it reads alert subject might not trigger notification if the subject was changed.

I don't remember the details anymore, originally I made this change 2 years ago. Haven't seen VictorOps since almost the same time.

jertel
jertel approved these changes Jul 21, 2021
View reviewed changes
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.

Thanks for the quick response, and for the PR contribution. Hoping you get some time off soon! :)

@jertel jertel merged commit 89c0234 into jertel:master Jul 21, 2021
@ChristophShyper ChristophShyper deleted the patch-3 branch July 21, 2021 13:59
@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.
Reviewers

@jertel jertel jertel approved these changes

@nsano-rururu nsano-rururu nsano-rururu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ChristophShyper @nsano-rururu @jertel