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

Adds escalation step to notify a team #3477

Closed
wants to merge 3 commits into from
Closed

Adds escalation step to notify a team #3477

wants to merge 3 commits into from

Conversation

xssfox
Copy link
Contributor

@xssfox xssfox commented Nov 30, 2023 •
edited
Loading

What this PR does

Adds a "Notify Team" escalation step which will notify all users in the assigned grafana team.

Screenshot 2023-12-01 at 10 35 47 am Screenshot 2023-12-01 at 10 36 10 am

Which issue(s) this PR fixes

Fixes #3407

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@xssfox xssfox force-pushed the add-notify-team branch 2 times, most recently from d3df5fb to 85558b6 Compare November 30, 2023 23:45
@xssfox xssfox marked this pull request as ready for review November 30, 2023 23:47
@xssfox xssfox requested a review from a team as a code owner November 30, 2023 23:47
@xssfox xssfox requested review from a team November 30, 2023 23:47
@xssfox xssfox marked this pull request as draft November 30, 2023 23:52
@xssfox xssfox marked this pull request as ready for review December 1, 2023 00:01
@iskhakov
Copy link
Contributor

iskhakov commented Dec 1, 2023

Thank you for the contribution! Great work!
We might use step "Notify Team" to notify team's escalation chains. Could you please consider renaming "Notify team" to something more specific, like "Notify all team members" or "Notify all users from team" in the code.

@xssfox
Copy link
Contributor Author

xssfox commented Dec 1, 2023

Thanks @iskhakov

I've updated the code to mostly be notify_team_members to keep the variables not too long and made the frontend changes a bit more clearer

image image

There's currently a test failing engine/apps/integrations/tests/test_views.py however it seems to be unrelated to my changes. I performed a make build, I suspect my branch doesn't have the required requirement for DjangoDbBlocker

@github-actions GitHub Actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the pr:stale Added to a PR that has been deemed "stale". Managed by the actions/stale GitHub Action label Jan 20, 2024
@xssfox
Copy link
Contributor Author

xssfox commented Feb 9, 2024

I think this PR is still valid however I'm not going to put effort into fixing up conflicts if there's no intention to merge

matiasb
matiasb reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

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

Thanks (again) for the contribution, I like the idea. Leaving a few comments I think we need to address before merging the changes. Happy to keep reviewing/discussing updates 👍

engine/apps/alerts/models/alert_group_log_record.py Outdated Show resolved Hide resolved
engine/apps/alerts/models/escalation_policy.py Outdated Show resolved Hide resolved
engine/apps/alerts/tasks/notify_team_members.py Outdated Show resolved Hide resolved
engine/apps/alerts/tasks/notify_team_members.py Outdated Show resolved Hide resolved
engine/apps/alerts/tasks/notify_team_members.py Outdated Show resolved Hide resolved
engine/apps/public_api/serializers/escalation_policies.py Show resolved Hide resolved
engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_policy_snapshot.py Show resolved Hide resolved
@github-actions github-actions bot removed the pr:stale Added to a PR that has been deemed "stale". Managed by the actions/stale GitHub Action label Feb 10, 2024
@xssfox
Copy link
Contributor Author

xssfox commented Feb 10, 2024

Thanks for the detailed comments, will have a look in the coming days :)

@xssfox
Copy link
Contributor Author

xssfox commented Feb 14, 2024

Thanks @matiasb, I've rebased to latest dev, added some tests and I believe fixed the issues above. I've also tested locally to make sure everything still works and it seems to. Let me know if there are any other changes that need looking at.

@xssfox xssfox requested a review from matiasb February 14, 2024 13:17
@matiasb
Copy link
Contributor

matiasb commented Feb 14, 2024

Thanks for the updates, I will be taking a look before the end of the week 👍

@matiasb matiasb mentioned this pull request Feb 16, 2024
Merged
@matiasb
Copy link
Contributor

matiasb commented Feb 16, 2024

Hi @xssfox! I hope you don't mind but it seems I'm not able to push updates to this PR, so forked it here to implement some additional refactoring and changes. Happy to share any details on the updates, let me know if that looks ok to you. Will get someone else to do a final review, but I think it should be ready to merge with that. Thanks for your contribution, and your patience 👍

@xssfox
Copy link
Contributor Author

xssfox commented Feb 16, 2024

@matiasb Looks good, thanks!

matiasb added a commit that referenced this pull request Feb 20, 2024
@matiasb @xssfox
Add escalation step to notify all members from a team (#3908)
d6467e9 
Based on #3477

---------

Co-authored-by: xssfox <xss@sprocketfox.io>
@xssfox xssfox closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matiasb matiasb Awaiting requested review from matiasb

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

Successfully merging this pull request may close these issues.

"Notify users" escalation should allow sending to a team
4 participants
@xssfox @CLAassistant @iskhakov @matiasb