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

[no sq] Changes #70

Merged
merged 7 commits into from
Feb 17, 2025
Merged

[no sq] Changes #70

merged 7 commits into from
Feb 17, 2025

Conversation

sfan5
Copy link
Contributor

@sfan5 sfan5 commented Feb 13, 2025

e

As discussed internally there are some edge cases where this
would require unreasonable changes on the server owner's part
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Looks good overall. Didn't test.
I would like to have this PR not squashed upon merge. The commits are separated very nicely. It would be a shame to lose that.

@@ -567,6 +588,13 @@ def get(self, ip, port):
i, server = self.getWithIndex(ip, port)
return server

def checkDuplicate(self, other_server):
Copy link
Member

Choose a reason for hiding this comment

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

isDuplicate would be a more accurate name, which is unfortunately lexically similar to is_duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer check here for reasons I can't really explain.

@SmallJoker SmallJoker changed the title Changes [no sq] Changes Feb 13, 2025
server.verifyLevel = 1

# do this here since verifyLevel is now set
if serverList.checkDuplicate(server):
Copy link
Contributor

Choose a reason for hiding this comment

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

This only returns true if it's a duplicate of a different server and that server is more trusted, so maybe this should be named something like isImpersonation instead.

Copy link
Contributor Author

@sfan5 sfan5 Feb 16, 2025

Choose a reason for hiding this comment

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

calling this "impersonation" feels heavy-handed when there are innocent reasons for this to happen, dunno

@sfan5 sfan5 force-pushed the chungus branch 3 times, most recently from fb3fb15 to 777ad8d Compare February 17, 2025 18:19
@sfan5 sfan5 merged commit de30a4e into master Feb 17, 2025
2 checks passed
@sfan5
Copy link
Contributor Author

sfan5 commented Feb 17, 2025

Now running in production.

@sfan5 sfan5 deleted the chungus branch February 17, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants