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

Webhooks notifications based on ENV vars #25

Merged
merged 10 commits into from
Jul 30, 2019

Conversation

tareksamni
Copy link
Contributor

Webhooks notifications based on ENV vars with unit tests covered.

Missing: Code documentation for new modules and classes

@brotandgames
Copy link
Owner

brotandgames commented Jul 30, 2019

Looks great 👍

Going to test the new feature in our staging environment with RocketChat.
If you are interested - this is done by: git push dokku tareksamni-master:master since we are currently using dokku for building and testing ciao.

Should we expand the placeholder possibilites with check_url and url?

Co-Authored-By: Brot & Games <43862266+brotandgames@users.noreply.github.com>
@tareksamni
Copy link
Contributor Author

Yes, I can do that. Should we add:

  1. check_url is the url that we ping to checkhealth.
  2. ciao_url is the "#show" link to the specific check that changed.

I was also thinking to move the CheckMailer call to a Notification of type MailNotification that implements #notify and use polymorphism. Then append an instance of it to NOTIFICATIONS in the initialiser. To be more consistent.

@brotandgames
Copy link
Owner

I would stay consistent here (with AR and the JSON API) and remove check_from check_name and

  • url is the url that we ping to checkhealth.
  • check_url is the "#show" link to the specific check that changed.

I was also thinking to move the CheckMailer call to a Notification of type MailNotification that implements #notify and use polymorphism. Then append an instance of it to NOTIFICATIONS in the initialiser. To be more consistent.

Sounds good 👍

Would only add that the Mailer should be added only if ENV['SMTP_ADDRESS'] is defined - suppose you were thinking of it.

@brotandgames
Copy link
Owner

brotandgames commented Jul 30, 2019

Tested with private RocketChat instance: works 👍

grafik

Will update example configuration for RocketChat in md.

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.

None yet

2 participants