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

Fix confused handling of replication ports in the DB #420

Merged
merged 4 commits into from Oct 12, 2021

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Oct 11, 2021

It's niche, but if no replication base url was configured and a peer was
configured with a NULL port in the db, then we would try to format None
as an integer and get a TypeError for our troubles.

Equally, if we did specify a port in the DB and did provide a replication URL, the db's port would be ignored---except for logging, which would happily report the wrong port.

Spotted by work in progress elsewhere to make Sydent pass mypy --strict.

Fixes #419.

It's niche, but if no replication base url was configured and a peer was
configured with a NULL port in the db, then we would try to format None
as an integer and get a TypeError for our troubles.

Spotted by work in progress elsewhere to make Sydent pass mypy --strict.

Fixes #419.
@DMRobertson DMRobertson changed the title Fix TypeError when peers.port contains nulls Fix confused handling of replication ports in the DB Oct 11, 2021
@DMRobertson DMRobertson requested a review from a team October 11, 2021 15:16
@DMRobertson DMRobertson marked this pull request as ready for review October 11, 2021 15:16
Copy link
Contributor

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but I think one of the logging updates is wrong.

sydent/replication/peer.py Outdated Show resolved Hide resolved
sydent/replication/pusher.py Outdated Show resolved Hide resolved
David Robertson and others added 2 commits October 12, 2021 19:48
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
I wonder if mypy would have spotted this---surprised flake8 didn't!
@DMRobertson DMRobertson merged commit f4e206a into main Oct 12, 2021
@DMRobertson DMRobertson deleted the dmr/remote-peer-port branch October 12, 2021 18:55
DMRobertson pushed a commit that referenced this pull request Oct 14, 2021
The type change was made in #420, and we don't want to rearrange the
handling of `port is None`.

This reverts commit b464459.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RemotePeer configuration has suspicious handling of port
2 participants