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

Don't overwrite self.teams upon parsing responders in OpsGenie integration #1539

Merged
merged 8 commits into from
Sep 27, 2024

Conversation

lstyles
Copy link
Contributor

@lstyles lstyles commented Sep 24, 2024

Description

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

@jertel
Copy link
Owner

jertel commented Sep 25, 2024

Thanks for submitting this. There are already extensive unit tests for this alerter, so adjusting one of them to send two alerts and then assert that the second one was unaffected by the first should (hopefully) be low-effort.

@lstyles
Copy link
Contributor Author

lstyles commented Sep 25, 2024

Thanks. Updated, but the changes needed were a bit more extensive than I would have liked.

I've added a test for this particular issue, but had to make some other changes to tidy things up a bit. Some of the tests were checking for (what I think) were incorrect values and some others were checking properties on the alert object instead of mocked post request.

I've done the recipients as well.

Let me know what you think. Apologies if it is a mess, I've not done any Python dev prior to this.

Comment on lines +38 to +41
if responders is None:
return None
if responder_args is None:
responder_args = dict()
Copy link
Contributor Author

@lstyles lstyles Sep 25, 2024

Choose a reason for hiding this comment

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

The top two lines prevent other tests from failing as missing responder_args no longer skips the execution.

The bottom two lines fix a scenario where responders contains a placeholder, but responders_args isn't configured in a rule at all. Previously, a text with placeholder would end up as a responder. Now, a warning will be shown and default_responders will be used instead.

@lstyles
Copy link
Contributor Author

lstyles commented Sep 25, 2024

The two new tests could probably be named to better describe what they're doing:

  • preserve_rule_config_upon_parsing_teams
  • preserve_rule_config_upon_parsing_recipients
    or something like that

@nsano-rururu
Copy link
Collaborator

The change from opsgenie_default_receipients to opsgenie_default_recipients requires the following documents to be updated:
https://github.com/jertel/elastalert2/blob/master/docs/source/alerts.rst

@lstyles
Copy link
Contributor Author

lstyles commented Sep 25, 2024

@nsano-rururu I've updated alerts.rst as well but only now realised it makes this a breaking change. I was looking at Elastalert(1) docs and saw the correct spelling there. Not sure what the right approach here is.

@nsano-rururu
Copy link
Collaborator

The next version, 2.21.0, will allow both the old setting name and the correct wording. However, if you use the old setting name, how about informing the user that the old setting value will be deleted when the logger reaches 2.25.0 every time an alert notification is sent?

@nsano-rururu
Copy link
Collaborator

For warning loggers, use elastalert_logger.warning.

example
https://github.com/jertel/elastalert2/pull/793/files

…elling and added backwards compatible handling of the old spelling
@lstyles
Copy link
Contributor Author

lstyles commented Sep 25, 2024

I've put something together but on alerter initialisation instead of alert send. Deprecation logic feels more in place there vs having it in the alert method.

Edit: Just realised you've got a mechanism for that already. 👀
Edit2: Not anymore. Looks like it was temporary, but also slightly different scenario and on a rule level instead of alerter level. 🤷

@nsano-rururu
Copy link
Collaborator

Update CHANGELOG.md.

@nsano-rururu
Copy link
Collaborator

@jertel

Any other comments?

@jertel
Copy link
Owner

jertel commented Sep 26, 2024

Looks good, thank you both. The schema.yaml is missing those two properties, as well as other OpsGenie properties. But that can wait for another PR since it's not directly related to this.

@lstyles
Copy link
Contributor Author

lstyles commented Sep 26, 2024

#1543 schema update as a separate PR (1 - to not block this one, 2 - because I'm not 100% sure it's correct)

@jertel jertel merged commit 46cb90d into jertel:master Sep 27, 2024
1 check passed
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.

3 participants