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 notification integration #158

Merged
merged 12 commits into from
Dec 9, 2019
Merged

Slack notification integration #158

merged 12 commits into from
Dec 9, 2019

Conversation

GaruGaru
Copy link
Contributor

As proposed in #101 this add the support for notifications with slack webhooks and also add a notifier interface that allows the implementation of other providers (as proposed by @linki ) with tests for both chaoskube integration and slack specific code.

The notifier is enabled by the --slack-webhook flag, the notifier itself is also wrapped by a Notifiers struct that allow the support for multiple notifiers at once (like prometheus alert manger notification system).

Result
notification-example

notifier/slack_test.go Outdated Show resolved Hide resolved
@linki
Copy link
Owner

linki commented Nov 12, 2019

@GaruGaru Thank you! That looks pretty complete. 😃

Do you mind if I push some slight changes here and there?

@GaruGaru
Copy link
Contributor Author

Hi @linki Feel free to make as many changes as needed !

@linki
Copy link
Owner

linki commented Nov 27, 2019

@GaruGaru I refactored it a bit but didn't change any behaviour.

@linki
Copy link
Owner

linki commented Nov 27, 2019

I changed the notification interface to take a Pod directly. I hope that's ok.

NotifyTermination(pod v1.Pod) error

@GaruGaru
Copy link
Contributor Author

Thank you for the feedback! Looks good to me, the only thing that I'm not sure about is the Notifier interface NotifyTermination method, in the future chaoskube may support node (or other resources) termination, so the NotifyTermination(pod v1.Pod) may be too specific I'd either change it to NotifyPodTermination(pod v1.Pod) or keep the NotifyTermination signature but change the parameter with a more generic one

@linki
Copy link
Owner

linki commented Nov 29, 2019

@GaruGaru I understand and it makes sense.

In this project I tried to keep things simple and don't think too far ahead even if it breaks the API later. So I'm in favour of NotifyPodTermination(pod v1.Pod) for now.

But honestly I'm fine with both, so let's do the version you prefer 😃

@GaruGaru
Copy link
Contributor Author

I get your point and thank you for letting me choose the method naming :D I think that NotifyPodTermination is more coherent with the method signature.

notifier/notifier.go Outdated Show resolved Hide resolved
@linki linki merged commit 3ec7983 into linki:master Dec 9, 2019
@linki
Copy link
Owner

linki commented Dec 9, 2019

@GaruGaru Thank you very much!

@linki
Copy link
Owner

linki commented Dec 9, 2019

Released in version v0.17.0.

@GaruGaru
Copy link
Contributor Author

GaruGaru commented Dec 9, 2019

Nice, Thank you very much !
We should also consider to update the Helm Chart with the new parameter in the future for the next releases, I can come up with a pull request if needed

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