Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

master/worker/task add strictly config verification #212

Merged
merged 4 commits into from
Jul 24, 2019
Merged

master/worker/task add strictly config verification #212

merged 4 commits into from
Jul 24, 2019

Conversation

3pointer
Copy link
Contributor

What problem does this PR solve?

Make sure all items in configuration file has been parsed and user always knows the exactly configuration about master worker and task.

What is changed and how it works?

  • For master worker(.toml) config, check whether undecoded item exists, and if exists, refuse to start server
  • For task(.yaml) config, use unmarshalStrict to decode and return error to caller when not all config item decoded and duplicate item existed
  • Add test
  • Remove useless config item in unit tests and integration tests

Check List

Tests

  • Unit test
  • Integration test

@3pointer 3pointer added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/feature New feature type/feature-request This issue is a feature request labels Jul 22, 2019
@codecov
Copy link

codecov bot commented Jul 22, 2019 •
edited
Loading

Codecov Report

Merging #212 into master will increase coverage by 0.3196%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master       #212        +/-   ##
================================================
+ Coverage   58.5971%   58.9167%   +0.3196%     
================================================
  Files           123        123                
  Lines         14057      14069        +12     
================================================
+ Hits           8237       8289        +52     
+ Misses         4990       4950        -40     
  Partials        830        830

suzaku
suzaku reviewed Jul 22, 2019
View reviewed changes
Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

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

LGTM

amyangfei
amyangfei reviewed Jul 23, 2019
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

Besides after this pr, old task config or DM instance config in user environment may not work because of historical deprecated configuration, we'd better update docs to record this after this pr is merged. https://github.com/pingcap/docs-cn/blob/master/dev/reference/tools/data-migration/dm-upgrade.md

dm/config/task.go
@@ -294,7 +294,7 @@ func (c *TaskConfig) DecodeFile(fpath string) error {
return errors.Annotatef(err, "read config file %v", fpath)
}

err = yaml.Unmarshal(bs, c)
err = yaml.UnmarshalStrict(bs, c)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can add one more test to cover DecodeFile with invalid yaml config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@3pointer
Copy link
Contributor Author

Besides after this pr, old task config or DM instance config in user environment may not work because of historical deprecated configuration, we'd better update docs to record this after this pr is merged. https://github.com/pingcap/docs-cn/blob/master/dev/reference/tools/data-migration/dm-upgrade.md

OK, And maybe ansible-playbook config templates also need to update? but I didn't find dm-ansible git repo.

@amyangfei
Copy link
Contributor

OK, And maybe ansible-playbook config templates also need to update? but I didn't find dm-ansible git repo.

it is in DM repo, dm/dm-ansible directory

@3pointer
Copy link
Contributor Author

OK, And maybe ansible-playbook config templates also need to update? but I didn't find dm-ansible git repo.

it is in DM repo, dm/dm-ansible directory

I have removed meta-file in dm-worker template in latest commit, other useless config seem to be removed already, please review it

amyangfei
amyangfei approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jul 23, 2019
@3pointer 3pointer merged commit aa39ff9 into pingcap:master Jul 24, 2019
@3pointer 3pointer deleted the config_verify branch July 24, 2019 02:22
@3pointer 3pointer mentioned this pull request Jul 24, 2019
Merged
4 tasks
@TomShawn TomShawn mentioned this pull request Aug 13, 2019
Merged
5 tasks
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@3pointer
master/worker/task add strictly config verification (pingcap#212)
e37f9cc 
* add task config verify

* remove useless test config

* add decodefile tests

* remove useless config in dm-ansible
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@suzaku suzaku suzaku left review comments

@amyangfei amyangfei amyangfei approved these changes

Assignees
No one assigned
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature type/feature-request This issue is a feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@3pointer @amyangfei @suzaku