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

docs: Clarifies default port of hosts #5290

Merged
merged 3 commits into from
Feb 15, 2025
Merged

docs: Clarifies default port of hosts #5290

merged 3 commits into from
Feb 15, 2025

Conversation

bthomee
Copy link
Collaborator

@bthomee bthomee commented Feb 14, 2025

High Level Overview of Change

The current comment in the example cfg file incorrectly mentions both "may" and "must". This change fixes this comment to clarify that the default port of hosts is 2459 and that specifying it is therefore optional. It further sets the default port to 2459 instead of the legacy 51235.

Type of Change

  • Documentation update

@bthomee bthomee added Trivial Simple change with minimal effect, or already tested. Only needs one approval. Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Feb 14, 2025
@bthomee bthomee requested a review from ximinez February 14, 2025 14:59
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (db0fad6) to head (1a47dce).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5290     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          791     791             
  Lines        68006   68006             
  Branches      8348    8350      +2     
=========================================
- Hits         52961   52959      -2     
- Misses       15045   15047      +2     

see 1 file with indirect coverage changes

Impacted file tree graph

@legleux
Copy link
Collaborator

legleux commented Feb 14, 2025

When I saw this port being used I didn't realize it wasn't arbitrary and wondered why it wasn't the default in the config.
IANA XRPL protocol port assignment

@ximinez
Copy link
Collaborator

ximinez commented Feb 14, 2025

This is more of a docs: than a chore:.

@bthomee
Copy link
Collaborator Author

bthomee commented Feb 14, 2025

This is more of a docs: than a chore:.

Fair - I was doubting between the two.

@bthomee bthomee changed the title chore: Clarifies default port of hosts docs: Clarifies default port of hosts Feb 14, 2025
@kennyzlei kennyzlei added this to the 2.4.0 (Q1 2025) milestone Feb 14, 2025
@bthomee bthomee merged commit 466849e into develop Feb 15, 2025
24 checks passed
@bthomee bthomee deleted the bthomee/portmay branch February 15, 2025 02:37
@legleux legleux mentioned this pull request Feb 18, 2025
5 tasks
@bthomee bthomee mentioned this pull request Feb 19, 2025
1 task
This was referenced Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. Trivial Simple change with minimal effect, or already tested. Only needs one approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants