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.

Sign up for GitHub

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

Jump to bottom

timeutil: do not use system files to verify timezone #13593

Merged
merged 7 commits into from
Nov 25, 2019
Merged

timeutil: do not use system files to verify timezone #13593

merged 7 commits into from
Nov 25, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Nov 19, 2019

Signed-off-by: Shuaipeng Yu [email protected]

What problem does this PR solve?

Use some system files to verify timezone making TiDB not portable.

What is changed and how it works?

Use time.LoadLocation to verify the timezone.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch

@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Nov 19, 2019
@jackysp jackysp requested a review from zhexuany November 19, 2019 12:13
@codecov
Copy link

codecov bot commented Nov 19, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13593   +/-   ##
===========================================
  Coverage   80.0622%   80.0622%           
===========================================
  Files           473        473           
  Lines        116362     116362           
===========================================
  Hits          93162      93162           
  Misses        15847      15847           
  Partials       7353       7353

@Deardrops
Copy link
Contributor

Deardrops commented Nov 20, 2019 •
edited
Loading

For getting system's time zone, You could use time.Local in "time" std pkg.
It does the same thing with InferSystemTZ function for getting system's time zone.
Also, time.Local.String() will return the string we need returned by InferSystemTZ.

Related PR #13614

@jackysp
Copy link
Member Author

jackysp commented Nov 21, 2019

Thanks for your suggestion, @Deardrops ! But I think it is not the key point of InferSystemTZ. I'm also confused about the current implementation. I need some help from the author.

@jackysp
Copy link
Member Author

jackysp commented Nov 22, 2019

/run-all-tests

@jackysp
fix test
ad6be45 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
Copy link
Member Author

jackysp commented Nov 22, 2019

/run-all-tests

@zyxbest
Copy link
Contributor

zyxbest commented Nov 25, 2019

/run-common-test

1 similar comment
@zyxbest
Copy link
Contributor

zyxbest commented Nov 25, 2019

/run-common-test

@jackysp jackysp requested review from coocood and lysu November 25, 2019 04:00
@coocood
Copy link
Member

coocood commented Nov 25, 2019

@breeswish PTAL

@jackysp
revert to the first version
2dbca08 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
fix comments
7000235 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
Copy link
Member Author

jackysp commented Nov 25, 2019

/run-all-tests

breezewish
breezewish reviewed Nov 25, 2019
View reviewed changes
Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

LGTM. @zhexuany PTAL

zhexuany
zhexuany approved these changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

LGTM

@zhexuany zhexuany added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Nov 25, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 25, 2019

/run-all-tests

@sre-bot sre-bot merged commit b0f534b into pingcap:master Nov 25, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@jackysp @XiaTianliang
timeutil: do not use system files to verify timezone (pingcap#13593)
6047102
@jackysp jackysp deleted the tz_portable branch February 27, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish left review comments

@zhexuany zhexuany zhexuany approved these changes

@coocood coocood Awaiting requested review from coocood

@lysu lysu Awaiting requested review from lysu

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jackysp @Deardrops @zyxbest @coocood @sre-bot @breezewish @zhexuany