-
Notifications
You must be signed in to change notification settings - Fork 25
Refactored discord notification code to not need to use chttp anymore #123
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
base: master
Are you sure you want to change the base?
Refactored discord notification code to not need to use chttp anymore #123
Conversation
Refactored discord notification code to not need to use chttp anymore.
BadgerCode
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.
Looks good! Thanks for doing this.
If we can default to using the existing convar for the webhook URL, it'll make updating easier for people.
But it's not the end of the world if we don't do it.
I can update the wiki.
| -- 2 - create messages for every report | ||
| Damagelog.DiscordWebhookMode = 0 | ||
| -- The URL of the webhook. You can create one in your Discord server settings. | ||
| Damagelog.DiscordWebhookURL = "" |
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.
Ah yeah good spot.
It was originally left out of the lua config, to avoid people accidentally sharing their discord webhook URLs.
But now, we have JSON config files which live outside of the code for the addon.
So the discord webhook URL config can live with all of the other config 🎉
| Damagelog.PrivateMessagePrefix = config.PrivateMessagePrefix | ||
|
|
||
| Damagelog.DiscordWebhookMode = config.DiscordWebhookMode | ||
| Damagelog.DiscordWebhookURL = config.DiscordWebhookURL |
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.
We might want to make the JSON config loader to pull any existing value from the console variable, for servers that already have it set up this way.
- Load the webhook URL from the JSON config file
- If the webhook URL is not set, try to load the webhook URL from the convar
ttt_dmglogs_discordurl
| if use_chttp then | ||
| HTTP = CHTTP | ||
| end | ||
|
|
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've tested this and it seems that discord have indeed unblocked requests from GMod servers!
(Or maybe the GMod server user agent changed).
Either way, yippee!
|
Okay, so, I did some research. I believe we could either a) scrap this, or b) make it work with these findings The problem here is that discord allows the messages, if sent by a windows server. It responds with code 403 if you try running the same on a linux machine (also tested it in containers and it gives the same result). So now either we scrap this (a) or change it so it checks if it can send a message and if yes just use HTTP and if no require the 3rd party DLL, |
|
Ah maybe I tested it wrong the other day. I'm using a Linux server. At the least, it would be nice to get the config changes into the project. |
Added config.DiscordWebhookURL / Damagelog.DiscordWebhookURL
Refactored discord notification code to not need to use chttp anymore.