-
Notifications
You must be signed in to change notification settings - Fork 42
feature: adds discord integration to GoMud #245
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
Conversation
|
@Volte6 We can customize the messages to your heart's desire easily, but I did want to hold off until I got your feedback. Take a look here at the types of messages we could create: https://leovoel.github.io/embed-visualizer/ |
| } | ||
|
|
||
| // Discord integration | ||
| if webhookUrl := os.Getenv("DISCORD_WEBHOOK_URL"); webhookUrl != "" { |
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.
As part of the pattern we've been following, we should make this part of the config:
https://github.com/Volte6/GoMud/blob/master/internal/configs/configs.go#L26-L104
https://github.com/Volte6/GoMud/blob/master/_datafiles/config.yaml
This does make me think ... checking env vars for overrides could be useful.
I'll merge this and make the adjustments in a new branch, though, since I also want to get some PlayerDespawn event fixes in place (call all registered handlers before the actual logoff occurs)
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.
How can we make this part of the config? It'd be submitted to version control right? Or are you considering config-overrides.yaml?
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.
The webhook url should be treated as a secret value.
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.
Yeah as an override, not in the repo config.yaml.
I'll also look into adding the option to define config values via env for secret injection.
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.
That sounds good. Just my 0.02, the config.yaml settings are very mud-centric. I think of these integrations more as plugin/extensions to the mud, not intrinsic to the mud itself, which is why I didn't put it there. Integrating a chat listener is not a core feature, maybe plugins.yaml or extensions.yaml? or a "plugins": [{ }] inside the config yaml?
These are just my 0.02, integrate it however you like.
Best,
Colin
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.
Yeah you're probably right. Maybe a secrets config makes more sense long term.
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.
It's easy to move if/when there are more opportunities for people to extend or add plugins. I really don't think it matters at the moment and we'll discover what makes sense as it's used.
JulianDavis
left a comment
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.
I know I'm late to the party, sorry, I'm just a passerby who wanted to offer some advice.
|
|
||
| color, err := strconv.ParseInt(hexColor, 16, 32) | ||
| if err != nil { | ||
| message := fmt.Sprintf("Invalid color specified, expected format #000000") |
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.
You're shadowing the message variable here and nothing is being formatted, so this could be rewritten to:
return errors.New("Invalid color specified, expected format #000000")although I would suggest printing the value on error:
return fmt.Errorf("Invalid color specified: %q, expected format #000000", message)|
|
||
| marshalled, err := json.Marshal(payload) | ||
| if err != nil { | ||
| message := fmt.Sprintf("Couldn't marshal discord message") |
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.
Similar to above, nothing formatted here. Could be refactored to be:
return errors.New("Couldn't marshal discord message")| client := &http.Client{} | ||
| response, err := client.Do(request) | ||
| if err != nil { | ||
| message := fmt.Sprintf("Couldn't send POST request to discord.") |
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.
Same as above, drop fmt.Sprintf for errors.New
| err := SendMessage(message) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return true |
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.
err := SendMessage(message)
return err == nil| message := fmt.Sprintf(":x: **%v** disconnected", user.Character.Name) | ||
| err := SendMessage(message) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return true |
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.
err := SendMessage(message)
return err == nil| return errors.New("Discord client was not initialized.") | ||
| } | ||
|
|
||
| if strings.HasPrefix(hexColor, "#") { |
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.
use strings.TrimPrefix
This PR adds a very simple discord integration to GoMud.
To use the integration:
DISCORD_WEBHOOK_URL=https://www.mywebhook.com ./gomudon linux).All feedback is encouraged and welcome, learning Golang slowly.