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

added notification support for discord #157

Merged
merged 24 commits into from
Oct 10, 2019

Conversation

wryonik
Copy link
Contributor

@wryonik wryonik commented Jun 9, 2019

Added Disocrd notification support using Slack-Compatible Webhook.
#144

@wryonik wryonik requested a review from fristonio June 9, 2019 00:13
Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

This is not how I would like to see this implemented. Like I told you on the slack, implement this using an interface. Create a general interface for notifier

type Notifer interface {
    SendNotification(message) error
}

Create a struct for each provider for the notification like discord, slack and irc and implement this interface for these providers.

type SlackNotificationProvider struct  {
    WebHookUrl string
}

func NewNotifier(providerType ProviderType) Notifier {
   if providerType == ProviderType.Slack
       return SlackNotificationProvider{
           WebHookUrl: "URL"
       }
   retun nil
}

func (p *SlackNotificationProvider) SendNotification(messag string) error {
   // code to send notification
    return nil
}

This is way more elegant than what you are currently trying to do.

pkg/notify/discord.go Outdated Show resolved Hide resolved
pkg/notify/slack.go Outdated Show resolved Hide resolved
@wryonik wryonik requested a review from fristonio June 20, 2019 20:04
pkg/notify/main.go Outdated Show resolved Hide resolved
pkg/notify/main.go Outdated Show resolved Hide resolved
pkg/notify/main.go Outdated Show resolved Hide resolved
pkg/notify/main.go Outdated Show resolved Hide resolved
@fristonio
Copy link
Member

Also resolve the conflicts in the pull request.

@fristonio
Copy link
Member

@shubhamgupta2956 show me a demo for this in the lab sometime today and get this merged.

…r slack as well as discord can be called from same function
pkg/notify/main.go Outdated Show resolved Hide resolved
pkg/notify/main.go Outdated Show resolved Hide resolved
pkg/notify/main.go Outdated Show resolved Hide resolved
Removed return statement from SendNotification function so that now it can send notification to discord if it fails to send it to slack
@wryonik wryonik requested a review from fristonio August 5, 2019 16:38
_examples/example.config.toml Outdated Show resolved Hide resolved
kokil87
kokil87 previously approved these changes Sep 28, 2019

//In the Discord notification provider it was using the same payload which was used for slack.
//By writing "/slack" in the discord WebHookURL, it execute Slack-Compatible Webhook
func NewNotifier(URL string, ProviderType ProviderTypeEnum) Notifier {
Copy link
Member

Choose a reason for hiding this comment

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

Validate URL here


func (req *Request) FillReqParams() error {
req.PostPayload = PostPayload{
Username: "Beast",
Copy link
Member

Choose a reason for hiding this comment

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

Put these values in notify/defaults.go and use from there.

@@ -28,3 +28,8 @@ ssh_key = "/home/vsts/.beast/secret.key"
url = "[email protected]:sdslabs/nonexistent.git"
name = "nonexistent"
branch = "nonexistent"

[[webhooks]]
url = "https://discordapp.com/api/webhooks/615970326093758464/Dui7gVZVIaGvdys87I9O2Gn9Bx3ssNkdgkxSf3etEXN0ClxlNYSeTflbJd0obO81a_m1"
Copy link
Member

Choose a reason for hiding this comment

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

Actual token kyu hai example toml me?

@@ -86,8 +86,7 @@ type BeastConfig struct {
AvailableSidecars []string `toml:"available_sidecars"`
GitRemote GitRemote `toml:"remote"`
JWTSecret string `toml:"jwt_secret"`
SlackWebHookURL string `toml:"slack_webhook"`
DiscordWebHookURL string `toml:"discord_webhook"`
Webhooks []Webhook `toml:webhook`
Copy link
Member

Choose a reason for hiding this comment

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

Name this NotificationWebhooks

@@ -216,6 +215,12 @@ func (config *GitRemote) ValidateGitConfig() error {
return nil
}

type Webhook struct {
Copy link
Member

Choose a reason for hiding this comment

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

NotificationWebhook

type Webhook struct {
URL string `toml:"url"`
ServiceName string `toml:"service_name"`
Status bool `toml:"status"`
Copy link
Member

Choose a reason for hiding this comment

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

What is meant by Status - true or false. Change the name to something more sensible - Active

@@ -530,7 +530,7 @@ func StartUndeployChallenge(challengeName string, purge bool) error {
notify.SendNotification(notify.Success, msg)
}

log.Infof("Notification for the event sent to slack.")
log.Infof("Notification for the event sent.")
Copy link
Member

Choose a reason for hiding this comment

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

Remove the f from Infof

@@ -15,3 +15,9 @@ const (
WarningColor NotificationColor = "#FF4500"
SuccessColor NotificationColor = "#32CD32"
)

const (
Copy link
Member

Choose a reason for hiding this comment

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

Change the file name to constants.go only, since it is inside the notify package we dont need the noise of notification in the name.

func NewNotifier(URL string, ProviderType ProviderTypeEnum) Notifier {
url, err := url.ParseRequestURI(URL)
if url == nil || err != nil {
fmt.Errorf("Invalid notification webhook URL")
Copy link
Member

Choose a reason for hiding this comment

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

return from here if there is an error.
Use error log for logging error.

type NotificationWebhook struct {
URL string `toml:"url"`
ServiceName string `toml:"service_name"`
Active bool `toml:"status"`
Copy link
Member

Choose a reason for hiding this comment

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

toml should also be active

@@ -28,3 +28,8 @@ ssh_key = "/home/vsts/.beast/secret.key"
url = "[email protected]:sdslabs/nonexistent.git"
name = "nonexistent"
branch = "nonexistent"

[[webhooks]]
Copy link
Member

Choose a reason for hiding this comment

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

Add example for slack too.
Change the field status to active

@fristonio fristonio merged commit 2d9c536 into master Oct 10, 2019
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