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

Slack notifier implementation #1

Merged
merged 14 commits into from
May 3, 2022
Merged

Slack notifier implementation #1

merged 14 commits into from
May 3, 2022

Conversation

joulei
Copy link
Collaborator

@joulei joulei commented Apr 26, 2022

Description

Adds main functionality for the slack notifier

Notes for reviewers

The notifier implementation parts from Boom version 0.8.0 which hasn't been released yet.
In order to run tests, a local version of boom 0.8.0 must be set up, e.g. {:boom_notifier, path: "../boom"},

Additionally, to run locally, you'll have to specify a slack_webhook_url as described in the readme, instructions are also provided for creating a new slack application and setting up a webhook.

Screenshots

image

@joulei joulei changed the title Initial commit Slack notifier implementation Apr 26, 2022
@joulei
Copy link
Collaborator Author

joulei commented Apr 27, 2022

Squashed newer commits for clarity,
Had to switch back to Mix.config module instead of Config as it was just introduced in Elixir v1.9.
Similar changes for stack trace, as it's being formatted differently across elixir versions.

@joulei joulei requested review from iaguirre88 and mcass19 April 27, 2022 12:36
lib/slack_client/http_adapter.ex Outdated Show resolved Hide resolved
test/boom_slack_notifier_test.exs Outdated Show resolved Hide resolved
@joulei
Copy link
Collaborator Author

joulei commented Apr 28, 2022

As discussed with @mcass19, I replaced the BoomSlackNotifier.SlackClient.HttpAdapter behavior return types with more generic ones, so that users can implement it around their desired HTTP clients.
Added a section on the readme and a moduledoc to document it.

Copy link

@mcass19 mcass19 left a comment

Choose a reason for hiding this comment

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

Great job 🚀 🚀

Leave some minor things. Some of them are more opinionated syntactic things, so feel free to ignore all what you want

README.md Outdated Show resolved Hide resolved
lib/boom_slack_notifier/slack_notifier.ex Outdated Show resolved Hide resolved
lib/boom_slack_notifier/slack_notifier.ex Outdated Show resolved Hide resolved
lib/boom_slack_notifier/slack_notifier.ex Outdated Show resolved Hide resolved
lib/boom_slack_notifier/slack_client/http_adapter.ex Outdated Show resolved Hide resolved
test/support/http_adapter_mock.ex Outdated Show resolved Hide resolved
test/boom_slack_notifier_test.exs Outdated Show resolved Hide resolved
test/boom_slack_notifier_test.exs Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@iaguirre88 iaguirre88 left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@joulei joulei force-pushed the slack-notifier branch from 94a7bb5 to 07d64ce Compare May 3, 2022 16:43
@joulei joulei merged commit 44e6370 into master May 3, 2022
@joulei joulei deleted the slack-notifier branch May 3, 2022 17:02
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