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

change default ports #289

Merged
merged 1 commit into from
Aug 6, 2024
Merged

change default ports #289

merged 1 commit into from
Aug 6, 2024

Conversation

eolesinski
Copy link
Contributor

@eolesinski eolesinski commented Jul 24, 2024

This PR addresses issue #97 by updating the port numbers used in the raft_test.cpp unit tests to a higher range (29800-29802) to avoid conflicts with commonly used ports.

Port conflicts with 5000 are frequent, especially with the docker-registry and Apple's AirPlay Receiver on MacOS. To address this issue, the default ports are updated to 298xx, following the convention outlined in #259.

Tested: tested by running ./scripts/test.sh

EDIT: This change now updates all test endpoints that use ports at 5xxx

@HalosGhost HalosGhost self-requested a review July 24, 2024 16:18
@HalosGhost
Copy link
Collaborator

HalosGhost commented Jul 24, 2024

It looks like there are a lot offew other tests which use 5xxx portnumbers; are you hoping to address those all in this PR as well?

In particular, the cfg files used by the integration test suites.

@eolesinski
Copy link
Contributor Author

@HalosGhost Yep - I can go through the other tests and make those changes

Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

T-ACK. This is useful when running on my local system since port 5000 is often occupied. I approve of these changes but second @HalosGhost 's comment that this should be replicated elsewhere in the code.

Signed-off-by: eolesinski <eo2454@columbia.edu>
Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

T-ACK. Updates the ports-used-for-test to be outside the range of typically used ports. MacOS AirPlay users rejoice.

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

T-ACK. Just swaps out the default ports we use to something less crowded. Hopefully defaults to causing far fewer headaches!

Thanks @eolesinski

@HalosGhost HalosGhost merged commit 4c91a6d into mit-dci:trunk Aug 6, 2024
7 checks passed
@eolesinski eolesinski deleted the port-changes branch August 6, 2024 15:53
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.

3 participants