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

api: fix unexpected error when set config to the same value #1960

Merged
merged 2 commits into from Nov 28, 2019

Conversation

@disksing
Copy link
Member

disksing commented Nov 21, 2019

Signed-off-by: disksing i@disksing.com

What problem does this PR solve?

Fix part of #1899

What is changed and how it works?

Check if the config item exists in any configuration.

Check List

Tests

  • Unit test
  • Manual test
Signed-off-by: disksing <i@disksing.com>
@disksing disksing added the area/api label Nov 21, 2019
@disksing disksing added this to the v4.0.0-beta milestone Nov 21, 2019
@disksing disksing requested review from lhy1024 and rleungx Nov 21, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 21, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@c71ec95). Click here to learn what that means.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1960   +/-   ##
=========================================
  Coverage          ?   78.03%           
=========================================
  Files             ?      170           
  Lines             ?    16886           
  Branches          ?        0           
=========================================
  Hits              ?    13177           
  Misses            ?     2693           
  Partials          ?     1016
Impacted Files Coverage Δ
server/api/config.go 62.71% <76.47%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c71ec95...0a11b93. Read the comment docs.

@rleungx

This comment has been minimized.

Copy link
Member

rleungx commented Nov 21, 2019

It seems config set location-labels "" still cannot work.

@disksing

This comment has been minimized.

Copy link
Member Author

disksing commented Nov 21, 2019

@rleungx That's a different problem :(
Changed description to "fix part of #1899"

@shafreeck

This comment has been minimized.

Copy link
Contributor

shafreeck commented Nov 25, 2019

Are the flag updated, found necessary here?

I prefer to update the configuration in a simpler way:

  1. Get the original config object conf
  2. Unmarshal the data to conf. This will merge the old and new values
  3. Return an error if there is, overwise, return OK
@disksing

This comment has been minimized.

Copy link
Member Author

disksing commented Nov 28, 2019

@shafreeck If client sends a wrong config, we will not know.

Copy link
Member

rleungx left a comment

LGTM

@nolouch

This comment has been minimized.

Copy link
Member

nolouch commented Nov 28, 2019

/merge

@sre-bot sre-bot added the CanMerge label Nov 28, 2019
@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Nov 28, 2019

/run-all-tests

@sre-bot sre-bot merged commit 8238f7f into pingcap:master Nov 28, 2019
8 checks passed
8 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-pd/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-pd/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-pd/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@disksing disksing deleted the disksing:fix-config branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.