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

main : check --config-check should has --config flag specified first. #11855

Merged
merged 5 commits into from
Aug 26, 2019

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Aug 24, 2019

What problem does this PR solve?

when start tidb with flag: 
 --config-check & **without** --config specified
it will succeed in config check regardless of what content the config file has. 

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.

@AilinKid AilinKid changed the title main : add --config-check check main : check --config-check flag should have --config flag specified first. Aug 24, 2019
@AilinKid AilinKid changed the title main : check --config-check flag should have --config flag specified first. main : check --config-check should has--config flag specified first. Aug 24, 2019
@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #11855 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #11855   +/-   ##
=========================================
  Coverage   81.562%   81.562%           
=========================================
  Files          435       435           
  Lines        94105     94105           
=========================================
  Hits         76754     76754           
  Misses       11863     11863           
  Partials      5488      5488

@AilinKid AilinKid added needs-cherry-pick-2.1 require-LGT3 Indicates that the PR requires three LGTM. labels Aug 24, 2019
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use MustNil?

Copy link
Contributor Author

@AilinKid AilinKid Aug 24, 2019

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.

a previous fail case

Copy link
Contributor

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.

Copy link
Contributor Author

@AilinKid AilinKid Aug 24, 2019

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.

@AilinKid AilinKid changed the title main : check --config-check should has--config flag specified first. main : check --config-check should has --config flag specified first. Aug 26, 2019
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@tangenta tangenta added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 26, 2019
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 26, 2019
@Deardrops
Copy link
Contributor

LGTM

@AilinKid AilinKid merged commit 60e4ffe into pingcap:master Aug 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 26, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Aug 26, 2019

cherry pick to release-2.1 failed

AilinKid added a commit to AilinKid/tidb that referenced this pull request Aug 26, 2019
…pingcap#11855)

* check --config-check flag should has --config flag specified first.
AilinKid added a commit to AilinKid/tidb that referenced this pull request Aug 26, 2019
…pingcap#11855)

* check --config-check flag should has --config flag specified first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants