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

Add duplicate detection to announcebuild #266

Merged

Conversation

FloatingMilkshake
Copy link
Collaborator

Prevents announcing an Insider build with /announcebuild if it has already been announced before. Can be overridden with the force_reannounce option. This acts as collision detection too.

PRing because this feels like a suspiciously simple change—did I miss anything important?

Copy link
Owner

@Erisa Erisa left a comment

Choose a reason for hiding this comment

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

This seems fine.

There could be the potential, depending how close together the commands are, that the delay having to pull from / push to Redis will be big enough that the conflict is allowed anyway. if you wanted to reduce those odds, you could store the latest build in memory as well.
That was something I discovered when doing the warning collision detection: the more time you spend on I/O before being able to check, the greater the window where there can be a conflict anyway.

@FloatingMilkshake
Copy link
Collaborator Author

That's fair. I tried to send /announcebuild twice rapidly when testing, and found that my second command was refused before the first one was even responded to—so it seems to work pretty quickly. But once the set fills up a little, maybe it'll become an issue? I think I will probably leave it how it is for now, and if it becomes an issue later I'll consider storing the latest build in memory

@FloatingMilkshake FloatingMilkshake merged commit dc9a948 into main Feb 7, 2025
2 checks passed
@FloatingMilkshake FloatingMilkshake deleted the milkshake/announcebuild-duplicate-detection branch February 7, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants