-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add ability to send notifications to slack #531
Conversation
The following files are not gofmt-ed. By commenting pkg/app/piped/notifier/slack.go--- pkg/app/piped/notifier/slack.go.orig
+++ pkg/app/piped/notifier/slack.go
@@ -213,7 +213,7 @@
func makeSlackMessage(title, titleLink, text, color string, timestamp int64, fields ...slackField) slackMessage {
return slackMessage{
Username: slackUsername,
- Attachments: []slackAttachment{slackAttachment{
+ Attachments: []slackAttachment{{
Title: title,
TitleLink: titleLink,
Text: text,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code coverage for golang is
|
Code coverage for golang is
|
pkg/app/piped/notifier/slack.go
Outdated
md := event.Metadata.(*model.EventDeploymentTriggered) | ||
title = fmt.Sprintf("Triggered a new deployment for %q", md.Deployment.ApplicationName) | ||
generateDeploymentEventData(md.Deployment) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these break
statements for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. Removed them.
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use |
Code coverage for golang is
|
} | ||
} | ||
|
||
switch event.Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, let me confirm that you didn't include intentionally some EventTypes such as EventType_EVENT_APPLICATION_SYNCED
etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Let me add a TODO for them in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you said adding in the next PR.
/approve |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
/cc @nakabonne