-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
main : check --config-check should has --config flag specified first. #11855
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11855 +/- ##
=========================================
Coverage 81.562% 81.562%
=========================================
Files 435 435
Lines 94105 94105
=========================================
Hits 76754 76754
Misses 11863 11863
Partials 5488 5488 |
// configCheck should have the config file specified | ||
if *configCheck { | ||
fmt.Fprintln(os.Stderr, "config check failed", errors.New("no config file specified for config-check")) | ||
os.Exit(1) |
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.
Could we use MustNil
?
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.
Em... it's not in test file .
This is same as previous config valid code when tidb start, Fprintln
to Stderr with err.
Cause user specified no config file, we new an error here.
Refer to this:
the right case, the fail case is what I have added.
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 know it isn't the test file, you can see MustNil
is called on line 347.
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.
Enen, got it. I've update my upper comment.
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.
LGTM
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.
LGTM
LGTM |
cherry pick to release-3.0 failed |
cherry pick to release-2.1 failed |
…pingcap#11855) * check --config-check flag should has --config flag specified first.
…pingcap#11855) * check --config-check flag should has --config flag specified first.
What problem does this PR solve?
What is changed and how it works?
Make sure --config is specified first when it has --config-check flag, then talk about file validation.
Otherwise, return config check failed cause no config file specified.
Check List
Tests
manual test
Code changes
modified in loadconfig.go
Side effects
no
Related changes
no
Release Note
check --config-check should have --config flag specified first.