-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Refactor IrcProvider just a little #1935
base: dev
Are you sure you want to change the base?
Conversation
Thank you for contributing to tgstation-server! The workflow 'CI Security' requires repository secrets and will not run without approval. Maintainers can add the |
src/Tgstation.Server.Host/Components/Chat/Providers/IrcProvider.cs
Outdated
Show resolved
Hide resolved
src/Tgstation.Server.Host/Components/Chat/Providers/IrcProvider.cs
Outdated
Show resolved
Hide resolved
newClient.OnReadLine += (sender, e) => Logger.LogTrace("READ: {line}", e.Line); | ||
newClient.OnWriteLine += (sender, e) => Logger.LogTrace("WRITE: {line}", e.Line); |
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.
Can you comment these two out again? They are really spammy for the logs in active channels.
newClient.OnReadLine += (sender, e) => Logger.LogTrace("READ: {line}", e.Line); | |
newClient.OnWriteLine += (sender, e) => Logger.LogTrace("WRITE: {line}", e.Line); | |
/*newClient.OnReadLine += (sender, e) => Logger.LogTrace("READ: {line}", e.Line); | |
newClient.OnWriteLine += (sender, e) => Logger.LogTrace("WRITE: {line}", e.Line);*/ |
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.
Is there any way we could make a config boolean to enable this?
from experience, it can be extremely helpful to diagnose issues.
having to compile from source for this could be fairly cumbersome esp. if trouble happens in prod.
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.
Commented in latest commit
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 could modify the GeneralConfiguration
to put this behind a flag.
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.
Actually, this is more appropriate for the FileLoggingConfiguration
src/Tgstation.Server.Host/Components/Chat/Providers/IrcProvider.cs
Outdated
Show resolved
Hide resolved
src/Tgstation.Server.Host/Components/Chat/Providers/IrcProvider.cs
Outdated
Show resolved
Hide resolved
src/Tgstation.Server.Host/Components/Chat/Providers/IrcProvider.cs
Outdated
Show resolved
Hide resolved
They said they're going to make the logging a config option. |
Pull request was converted to draft
🆑 Core
Refactors the IrcProvider to be a bit more resilient.
Fixes the IrcProvider getting permanently stuck if the connection between TGS and IRC is ever severed.
Adds the OPER Irc authentication mode, helpful for anyone running their own IRC servers.
/🆑
🆑 Nuget: API
Adds IrcPasswordType.Oper for /OPER authentication
/🆑
🆑 HTTP API
Adds new IRC authentication type with index 3 for /OPER authentication
/🆑
Related to #1085 but I don't think it should close it.
IrcProvider would break entirely if it ever got disconnected, stuck in a weird infinite loop of trying to connect but aborting of its own accord. This solves that issue by minting a new IrcFeatures client every time a connection attempt is made.
Additionally, the implementation was slightly flaky and would emit useless and broken bytes at the start of every session, this confuses irc servers and should be avoided. IrcProvider will now actually wait for a successful registration instead of just absolutely bean blasting the server and hoping it works.
The test has been amended to wait and check to ensure the IrcProvider actually stays connected to a server for atleast 2 seconds, and does this test twice. This amended test would have caught the issue in the original IrcProvider.
Also adds a new authentication mode OPER which, as the name implies will do an IRC /OPER at connect. Maybe i'm just the only weirdo who needs that, if so, i'll gladly strip it out if needed. My only justification is that I run a irc server without services and just use the +O (ircop only) channel mode to restrict people getting into my admin channel.
Tested over 2 days, atleast from what I can see is fully functioning and not causing any issues.