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

fix drainer initialCommitTs issue #2857

Merged
merged 10 commits into from
Jul 17, 2020

Conversation

weekface
Copy link
Contributor

@weekface weekface commented Jul 2, 2020

What problem does this PR solve?

initialCommitTs should be a string, itherwise failed, for example: initialCommitTs: 417774109500375041 will be parsed as:

image

What is changed and how does it work?

Check List

Tests

  • Unit test
  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has Go code change
  • Has CI related scripts change
  • Has Terraform scripts change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:

Fix drainer values.yaml `initialCommitTs` bug

cofyc
cofyc previously approved these changes Jul 2, 2020
cofyc
cofyc previously approved these changes Jul 6, 2020
DanielZhangQD
DanielZhangQD previously approved these changes Jul 6, 2020
Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielZhangQD DanielZhangQD self-requested a review July 6, 2020 09:24
@weekface weekface dismissed stale reviews from DanielZhangQD and cofyc via 3d59b78 July 6, 2020 09:36
@weekface weekface requested a review from cofyc July 6, 2020 09:37
Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielZhangQD
Copy link
Contributor

@cofyc PTAL.

@cofyc
Copy link
Contributor

cofyc commented Jul 17, 2020

/merge

@ti-srebot
Copy link
Contributor

@cofyc Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2020

Codecov Report

Merging #2857 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2857      +/-   ##
==========================================
- Coverage   40.91%   40.89%   -0.02%     
==========================================
  Files         153      153              
  Lines       16748    16748              
==========================================
- Hits         6852     6849       -3     
- Misses       9354     9356       +2     
- Partials      542      543       +1     
Flag Coverage Δ
#unittest 40.89% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@DanielZhangQD DanielZhangQD merged commit 53d25ae into pingcap:master Jul 17, 2020
ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 17, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-1.0 in PR #2983

ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 17, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-1.1 in PR #2984

ti-srebot added a commit that referenced this pull request Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants