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

checker: allow to ignore some checking items #65

Merged
merged 8 commits into from
Mar 7, 2019
Merged

checker: allow to ignore some checking items #65

merged 8 commits into from
Mar 7, 2019

Conversation

IANTHEREAL
Copy link
Collaborator

@IANTHEREAL IANTHEREAL commented Mar 5, 2019

What problem does this PR solve?

allow to ignore some checking items

What is changed and how it works?

add a IgnoreCheckingItems configuration, and use it to filter checking items in checker

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    • set up a dm cluster
    • start a config by task
    • check output in log

Code changes

  • Has exported variable/fields change

@IANTHEREAL IANTHEREAL 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/enhancement Performance improvement or refactoring labels Mar 5, 2019
@IANTHEREAL
Copy link
Collaborator Author

/run-all-tests

checker/checker.go Outdated Show resolved Hide resolved
}
delete(checkingItems, AllChecking)

c.Assert(FilterCheckingItems(ignoredCheckingItems[:0]), DeepEquals, checkingItems)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
c.Assert(FilterCheckingItems(ignoredCheckingItems[:0]), DeepEquals, checkingItems)
c.Assert(FilterCheckingItems(nil), DeepEquals, checkingItems)

more readable?

@csuzhangxc
Copy link
Member

@july2993 PTAL

Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

rest LGTM

return nil
}

checkingItems := config.FilterCheckingItems(cfgs[0].IgnoreCheckingItems)
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear why just get from index 0

@IANTHEREAL
Copy link
Collaborator Author

@july2993 @csuzhangxc PTAL again

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc 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 Mar 7, 2019
@IANTHEREAL IANTHEREAL merged commit 8f72399 into pingcap:master Mar 7, 2019
@IANTHEREAL IANTHEREAL deleted the ian/check branch March 7, 2019 10:14
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants