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

Refactor IrcProvider just a little #1935

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

craftxbox
Copy link

@craftxbox craftxbox commented Sep 20, 2024

🆑 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
/🆑

🆑 Configuration
The new configuration version is 5.3.0.
Adds a new ProviderNetworkDebug flag in FileLogging to debug IRC traffic.
/🆑

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.

Copy link
Contributor

Thank you for contributing to tgstation-server! The workflow 'CI Security' requires repository secrets and will not run without approval. Maintainers can add the CI Cleared label to allow it to run. Note that any changes to ci-security.yml and ci-pipeline.yml will not be reflected.

@tgstation-server-ci tgstation-server-ci bot added the CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution label Sep 20, 2024
@Cyberboss Cyberboss self-assigned this Sep 20, 2024
@Cyberboss Cyberboss added this to the v6.11.0 milestone Sep 20, 2024
@Cyberboss Cyberboss added Refactor Refactor functionality for future improvements Feature New functionality Area: Chat With regard to managing chat bots labels Sep 20, 2024
@tgstation-server-ci tgstation-server-ci bot added size/L CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution and removed CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution labels Sep 20, 2024
Cyberboss
Cyberboss previously approved these changes Sep 21, 2024
@Cyberboss
Copy link
Member

They said they're going to make the logging a config option.

@Cyberboss Cyberboss marked this pull request as draft September 21, 2024 22:59
auto-merge was automatically disabled September 21, 2024 22:59

Pull request was converted to draft

@craftxbox craftxbox marked this pull request as ready for review October 2, 2024 22:07
@tgstation-server-ci tgstation-server-ci bot added CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution and removed CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution labels Oct 2, 2024
Copy link
Member

@Cyberboss Cyberboss left a comment

Choose a reason for hiding this comment

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

  • Add this as the first line to your Configuration changelog
**The new configuration version is 5.3.0.**
  • Configuration needs a minor version bump in build/Version.props
  • Merge upstream/dev to grab build fixes

Cyberboss
Cyberboss previously approved these changes Oct 4, 2024
@Cyberboss Cyberboss added the CI Cleared Apply this to a pull request from a fork to allow it to run the CI suite label Oct 4, 2024
@tgstation-server-ci tgstation-server-ci bot removed the CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution label Oct 4, 2024
auto-merge was automatically disabled October 4, 2024 03:41

Head branch was pushed to by a user without write access

@tgstation-server-ci tgstation-server-ci bot added CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution and removed CI Cleared Apply this to a pull request from a fork to allow it to run the CI suite labels Oct 4, 2024
@Cyberboss Cyberboss added the CI Cleared Apply this to a pull request from a fork to allow it to run the CI suite label Oct 4, 2024
@tgstation-server-ci tgstation-server-ci bot removed the CI Approval Required CI Pipeline execution blocked. Maintainers, apply the "CI Cleared" label to allow execution label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Chat With regard to managing chat bots CI Cleared Apply this to a pull request from a fork to allow it to run the CI suite Feature New functionality Refactor Refactor functionality for future improvements size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants