-
Notifications
You must be signed in to change notification settings - Fork 143
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
[#768] [Follow Up] feat(cli): Cli method for blacklist update. #968
Conversation
Codecov Report
@@ Coverage Diff @@
## master #968 +/- ##
============================================
- Coverage 54.93% 54.27% -0.67%
- Complexity 2055 2479 +424
============================================
Files 327 355 +28
Lines 15203 17991 +2788
Branches 1174 1726 +552
============================================
+ Hits 8352 9764 +1412
- Misses 6382 7625 +1243
- Partials 469 602 +133
... and 71 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
true, "This is coordinator server port."); | ||
ssl = new Option(null, longPrefix + "ssl", false, "use SSL"); | ||
ssl = new Option("l", longPrefix + "ssl", false, "use SSL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the short option is omitted on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@advancedxy Thank you for your help in reviewing the code! I tried to remove the short option and test it in the test environment. I think your feedback is accurate. It is better to use the long option (ssl).
@advancedxy Can you help review this pr again? Thank you very much! |
lgtm, would you mind to update the pr summary to reflect the short option of ssl cli arg? |
Thank you for your suggestion. I will modify the PR summary accordingly. |
@advancedxy @jerqi Can you help to review this PR again? |
What changes were proposed in this pull request?
While reading the code, I found some minor issues, this PR will fix them.
uniffle admin-cli
command without any options, it should print the help information.Why are the changes needed?
Improve code readability and enhance code functionality.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Test environment verification.