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

feat: Added validation for --client-listen-urls and --peer-listen-urls #698

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

ChaudharyRaman
Copy link
Contributor

This PR resolved this issue: #691
Please briefly answer these questions:

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
    During the startup process of Xline, certain essential command-line parameters such as --member and --storage-engine are checked. and there is a lack of validation for --client-listen-urls and --peer-listen-urls
    Here's how it looks -
    fix_cli
    fix--help

  • what changes does this pull request make?

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)

Copy link

codecov bot commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.39%. Comparing base (75e51f4) to head (cb0fbb0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   74.38%   74.39%   +0.01%     
==========================================
  Files         173      173              
  Lines       25872    25872              
  Branches    25872    25872              
==========================================
+ Hits        19244    19248       +4     
+ Misses       5425     5416       -9     
- Partials     1203     1208       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChaudharyRaman
Copy link
Contributor Author

@Phoenix500526 @iGxnon Please review this PR. I closed the previous one and remove unnecessary commits and also just wanted to push from proper PR branch name from my local. Please tell me if there is anytime more to be done for this PR.

@ChaudharyRaman
Copy link
Contributor Author

@Phoenix500526 is their any order in which a particular arguments needed to be shown, like right now --client-listen-urls and --peer-listen-urls are first and then rest of the flags, i will change if required.
While playing with Xline, i found a small ip-address issue with the quick_start.sh script for prometheus and quick-start DOC. Should i raise a issue for this or are we going to change the quick_start soon in this issue #692.

Copy link
Collaborator

@Phoenix500526 Phoenix500526 left a comment

Choose a reason for hiding this comment

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

LGTM

@Phoenix500526
Copy link
Collaborator

@Phoenix500526 is their any order in which a particular arguments needed to be shown, like right now --client-listen-urls and --peer-listen-urls are first and then rest of the flags, i will change if required. While playing with Xline, i found a small ip-address issue with the quick_start.sh script for prometheus and quick-start DOC. Should i raise a issue for this or are we going to change the quick_start soon in this issue #692.

Is the issue you mentioned above described in issue #692? If not, I think it's better to initiate a new issue to describe it.

@ChaudharyRaman
Copy link
Contributor Author

Thanks @Phoenix500526 .Got it! I will raise a issue for this although its just a minor fix.

@mergify mergify bot merged commit e56a82d into xline-kv:master Mar 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants