Mattermost incoming event handler #5273
Conversation
Remove print Add migrations
ravishankar15
force-pushed
the
mattermost-incoming-event-handler
branch
from
c8f2f84
to
26520ca
Compare
|
Hi @matiasb Please take a look at the incoming event handler changes for the alert message actions from mattermost. |
ravishankar15
temporarily deployed
to
github-pages
GitHub Actions
Inactive
ravishankar15
temporarily deployed
to
github-pages
GitHub Actions
Inactive
ravishankar15
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| @@ -94,6 +95,7 @@ def _make_actions(id, name, token): | |||
| "url": create_engine_url("api/internal/v1/mattermost/event/"), | |||
| "context": { | |||
| "action": id, | |||
| "alert": self.alert_group.pk, | |||
There was a problem hiding this comment.
It may be better to use the public primary key here instead
| return | ||
|
|
||
| def _get_alert_group(self) -> typing.Optional[AlertGroup]: | ||
| return self._get_alert_group_from_event() or self._get_alert_group_from_message() |
There was a problem hiding this comment.
Why do we need both ways to get the alert group? (shouldn't we always have the event context?)
There was a problem hiding this comment.
Yup first had only the message thing later added the context implementation should have removed the message thing. I don't see a need to have both
| handler = AlertGroupActionHandler(event=event, user=user) | ||
| handler.process() | ||
| alert_group.refresh_from_db() | ||
| assert alert_group.acknowledged |
There was a problem hiding this comment.
Should the action_source in the log record be set to Mattermost?
|
|
||
| token = MattermostEventAuthenticator.create_token(organization=organization) | ||
| event = make_mattermost_event( | ||
| EventAction.ACKNOWLEDGE, |
There was a problem hiding this comment.
It would be nice to also check other actions end with the expected alert group state too (parametrizing the test?).
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_mattermost_alert_group_event_acknowledge( |
There was a problem hiding this comment.
Double-checking, all these tests should be exercising the actions handler too, right?
There was a problem hiding this comment.
Yup it does, I have parameterised the event tests as well.
| response = client.post(url, event, format="json") | ||
| alert_group.refresh_from_db() | ||
| assert not alert_group.acknowledged | ||
| assert not alert_group.resolved |
There was a problem hiding this comment.
maybe check for alert_group.state for the expected final state instead?
|
Hi @matiasb I have addressed the review comments |
What this PR does
Handles the incoming event from the alert group message
Which issue(s) this PR closes
Related to [issue link here]
Checklist
pr:no public docsPR label added if not required)release:). These labels dictate how your PR willshow up in the autogenerated release notes.