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

[HOTFIX] Antag ban fix #35308

Closed
wants to merge 4 commits into from

Conversation

Killerqu00
Copy link
Contributor

About the PR

fixes #35266
hopefully I am doing a hotfix properly this time

Why / Balance

Technical details

Nukeops isn't a job

Media

server in dev doesn't crash anymore so that's a win, I guess

Requirements

Breaking changes

Changelog

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted Branch: Stable Intended to be merged into Stable. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/XS Denotes a PR that changes 0-9 lines. labels Feb 19, 2025
@Errant-4 Errant-4 added T: Bugfix Type: Bugs and/or bugfixes P1: High Priority: Higher priority than other items, but isn't an emergency. D3: Low Difficulty: Some codebase knowledge required. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. A: Admin Tooling Area: Admin tooling and moderation. T: Of Admin Interest Type: Affects administration work a lot, and might require admins to weigh in on and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 19, 2025
@slarticodefast
Copy link
Member

Why are both of these prototypes handled by a single id string by the way? That looks a little sus.

@Killerqu00
Copy link
Contributor Author

Why are both of these prototypes handled by a single id string by the way? That looks a little sus.

wish i knew

@Errant-4 Errant-4 added the Intent: Hotfix PR Intent: Something that should be applied via hotfix procedure. label Feb 20, 2025
@Errant-4
Copy link
Member

Errant-4 commented Feb 22, 2025

Updating severity to P0 Critical, as per issue

@Errant-4 Errant-4 added P0: Critical Priority: Servers-on-fire important, when it needs to be merged yesterday, or NOW. and removed P1: High Priority: Higher priority than other items, but isn't an emergency. labels Feb 22, 2025
Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

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

This doesn't make any sense. The function is still adding the Job: prefix to the role name, so this would just create invalid database data.

@PJB3005 PJB3005 added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Feb 23, 2025
@PJB3005 PJB3005 deleted the branch space-wizards:stable February 23, 2025 01:15
@PJB3005 PJB3005 closed this Feb 23, 2025
@PJB3005 PJB3005 reopened this Feb 23, 2025
@Killerqu00
Copy link
Contributor Author

Killerqu00 commented Feb 23, 2025

This doesn't make any sense. The function is still adding the Job: prefix to the role name, so this would just create invalid database data.

the antag bans aren't even real from the looks of it (#29730 is closed). i can just make it so it ignores the antag bans instead of throwing in order to prevent database pollution

@Killerqu00 Killerqu00 requested a review from PJB3005 February 23, 2025 10:30
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Feb 23, 2025
Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

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

This should be getting validated (and logged) by the UI that allows admins to place bans instead.

@PJB3005
Copy link
Member

PJB3005 commented Feb 23, 2025

Given that antag bans clearly aren't even a thing at all I'm gonna change the original issue description and lower the priority. I suspect the "server crashes" is just a thing caught by exception tolerance so it's not too important.

Probably best to make this not a hotfix anymore but you can do whateevr.

@PJB3005 PJB3005 added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. and removed P0: Critical Priority: Servers-on-fire important, when it needs to be merged yesterday, or NOW. labels Feb 23, 2025
@Killerqu00
Copy link
Contributor Author

sure, I'll make a separate PR for this then

@Killerqu00 Killerqu00 closed this Feb 23, 2025
@Killerqu00 Killerqu00 deleted the antag-ban-hotfix branch February 23, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Admin Tooling Area: Admin tooling and moderation. Branch: Stable Intended to be merged into Stable. D3: Low Difficulty: Some codebase knowledge required. Intent: Hotfix PR Intent: Something that should be applied via hotfix procedure. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted size/XS Denotes a PR that changes 0-9 lines. T: Bugfix Type: Bugs and/or bugfixes T: Of Admin Interest Type: Affects administration work a lot, and might require admins to weigh in on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants