-
Notifications
You must be signed in to change notification settings - Fork 997
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
Enhance Slack alerts #1415
base: main
Are you sure you want to change the base?
Enhance Slack alerts #1415
Conversation
Mention author
Mention author
Make slack mention actually link
Make problem.whodunnit return array regardless of size
Mention author
Remove GITHUB_TO_SLACK_USERS hash and pass as env variable instead
Rename GITHUB_TO_SLACK_USERS to SLACK_USER_ID_MAP
Fix bug in assigned_to logic
Adding affected user to slack notification
Support mentioning Slack user groups + add order ID to user attributes shown
Adding host name to slack notification
This is super cool @laasem. There are a lot of things our users want to do with notifications and my thought is Errbit is never going to be able to do all of them. With the amount of energy we have behind this project, I think the best thing we can do is package what you have created as a microservice (maybe a little sinatra app) and integrate it with Errbit using the 'webhook' notification system. I'd be happy to help with that if you want because then I'll at least have an answer for users who want to do all kinds of fancy stuff. Also, if it turns out our webhook system needs more features, we can enhance that as well. |
@stevecrozz That sounds great! In that case, I'm guessing it would be better to have it work for all the notification services Errbit currently supports, not just Slack? |
We can discuss different ideas. But my first thought is we should create a 'errbit_notifier_slack' app whose whole job is to accept a web hook from Errbit, build a suitable notification and then send it to slack. We can build one first and then if you still have energy you're certainly welcome to do the others. I think we could take most of this code and package it in this way, add a Dockerfile and push it to dockerhub. |
@stevecrozz Sorry for the extremely late response and thanks for your feedback! I can start working on packaging this as a separate app if there's still a need for it. |
It's totally up to you. Does the webhook payload from Errbit give you everything you need? |
@stevecrozz It has useful data about the I'm also not sure how much of the |
@laasem Adding to the web hook seems like a good idea. If you make a PR to add all the attributes you need to the web hook and a test or two, I'd be happy to merge that. As far as duplication goes, it wouldn't bother me at all if you copy/pasted all the code you need into errbit_slack_plugin and modify it however it makes sense. IMO it wouldn't be worth the effort to share any of this logic. |
Changes Slack alerts in 3 ways:
A more detailed discussion of these changes and the motivations behind them can be found here.
I have made sure there are no regressions by updating
./spec/models/notification_service/slack_service_spec.rb
accordingly and have noticed by observing other PRs that new tests are usually added for new routes rather than for new model helper methods, but if I need to add tests for each of the model helper methods that this PR adds do let me know.Update: Travis CI builds seem to fail because the env authentication variable
GITHUB_ACCESS_TOKEN
isn't passed. Not sure how to fix; have tried not sending GraphQL request if not passed but to no avail. Any help would be much appreciated.